Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ImageGallery): adds ImageGallery and ImageGalleryImage component for gallery view of photos #315

Merged
merged 45 commits into from
Aug 21, 2019
Merged

feat(ImageGallery): adds ImageGallery and ImageGalleryImage component for gallery view of photos #315

merged 45 commits into from
Aug 21, 2019

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Aug 7, 2019

Closes #229

Adds two components, ImageGallery and ImageGalleryImage, and their styles, to create the gallery view that exists in the IBM design language site. This also adds a page to the gatsby theme carbon site that provides a ten photo example of the Image Gallery in use. The gallery ideally matches 1-1 with the IBM Design Language Gallery

New

ImageGallery: image gallery works by creating a portal that contains the gallery view when an image is clicked.

ImageGalleryImage: this is a how images that will be in a gallery are rendered to the page. Based on whether or not the image is in a dialog, the image is either rendered in a column amongst a collection of images or is blown up into a full page view, allowing the users to click or arrow through all the images on the page. Currently requires the images to be in the static folder to render properly.

ImageGallery mdx file: provides an example of how the ImageGallery works and what it looks like used in markdown files.

Example with Now deployment: https://gatsby-theme-c-git-fork-abbeyhrt-229-feature-imade-galle-6f06a5.carbon-design-system.now.sh/components/ImageGallery

@vercel
Copy link

vercel bot commented Aug 7, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://gatsby-theme-c-git-fork-abbeyhrt-229-feature-imade-galle-6f06a5.carbon-design-system.now.sh

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks awesome! I have a few comments so far, will finish looking through this tomorrow.

I have an idea to fix the spacing issue w/ the right arrow, but might take a little tweaking.

Wondering since the tablet breakpoints aren't on the grid if we just want to have a col={4} instead of md and lg and then just add css to make it look like it stays 12 columns at tablet?

@vpicone
Copy link
Contributor

vpicone commented Aug 8, 2019

This isn’t working on mobile for me, have you started taking a look at that? Maybe just disabling it?

@alisonjoseph
Copy link
Member

@vpicone yep intended behavior at mobile is to disable it. Looks like its doing something weird though right now.

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Aug 8, 2019

I added in the spacing tokens and fixed the mobile weirdness(hopefully). I actually fixed the problem with the right arrow, where it was outside of the clickable area, it doesn't click all the way to the edge of the page but neither does the IDL site, it has the 32px margin that isn't clickable. Definitely can change that if we want to though!

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Aug 8, 2019

@Alison would you be able to help me/give pointers on how to make it look like it stays at 12 columns at tablet? Not super familiar with how to edit the columns like that.

@abbeyhrt
Copy link
Contributor Author

@jeanservaas I moved the title up and fixed the image not being centered, let me know if anything else needs to be changed!

}

.gallery-row {
// position: absolute;
Copy link
Contributor

@vpicone vpicone Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's take these comments out

@vpicone vpicone self-requested a review August 16, 2019 19:26
Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take away the button and figure wrappers, we can allow images to be passed in using the markdown syntax. This allows us to share syntax alt/src with other image components, not rely on the static directory, and take advantage of gatsby-image features such as image compression and blur-up.

Still need to replace the styling that was taken out by removing the wrappers but this should get you started!

function ImageGalleryImage({
  children,
  title,
  col,
  isInDialog = false,
  ...rest
}) {
  if (isInDialog) {
    return (
      <>
        <h3 className={imageTitle}>{title}</h3>
        {children}
      </>
    );
  }

  return (
    <Column colLg={col}>
      <div className={imageButtonWrapper} type="button" {...rest}>
        {children}
      </div>
    </Column>
  );
}

Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like arrows are broken on firefox. They're moved up to the top:

Screen Shot 2019-08-16 at 2 27 59 PM

@vercel vercel bot temporarily deployed to staging August 16, 2019 22:40 Inactive
@abbeyhrt
Copy link
Contributor Author

@vpicone Awesome, thank you! I updated the ImageGallery so it can use the gatsby markdown images now. I was able to keep the button and figure wrappers, so thankfully we can keep the interactive and a11y stuff they provide. I fixed the problem the arrows were having in firefox as well.

@abbeyhrt abbeyhrt requested a review from vpicone August 16, 2019 22:43
@vpicone
Copy link
Contributor

vpicone commented Aug 16, 2019

@abbeyhrt Wow! Looking great! That blur up is so nice. Last couple of things:

  1. It looks like you're doubling up with a margin on both the button and the image.

IDL

Screen Shot 2019-08-16 at 6 38 15 PM

This PR

Screen Shot 2019-08-16 at 6 38 12 PM

  1. Can we add a focus to override the default? Test on firefox as well cause it's getting an outline and a border (this might partially be fixed with the margin thing above)

Screen Shot 2019-08-16 at 6 35 03 PM

@jeanservaas
Copy link
Contributor

@abbeyhrt

please don't kill me, there were multiple alignment issues even on IDL gallery page... I told you to move up the gallery image name... but then the "x" looked weirdly off.

just consulted with pete garvin about it and I think we should follow the below spec — it's OK to keep the title of the gallery image lower I think so that it can properly align with the close 'x.' Super sorry to lead you in circles!

image

Copy link
Contributor

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alisonjoseph alisonjoseph merged commit eb31926 into carbon-design-system:master Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Image Gallery
4 participants