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: add ArtDirection component #294

Merged
merged 3 commits into from
Aug 1, 2019

Conversation

vpicone
Copy link
Contributor

@vpicone vpicone commented Jul 31, 2019

No description provided.

@vercel
Copy link

vercel bot commented Jul 31, 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-carbon-git-fork-vpicone-artdirection.carbon-design-system.now.sh

@alisonjoseph
Copy link
Member

Not sure about the name "ArtDirection", but otherwise love the simplicity here

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me! so are we only supporting a max of 3 different sizes to start?

@vpicone
Copy link
Contributor Author

vpicone commented Jul 31, 2019

@emyarod Yeah, otherwise you might end up duplicating the middle one to hit two media queries or something silly like that.

@alisonjoseph
Copy link
Member

If someone puts only two images the first will show at mobile, and the second at tablet and above, correct?

@vpicone
Copy link
Contributor Author

vpicone commented Jul 31, 2019

@alisonjoseph art direction is the actual name for the thing we're doing here.

I considered more vague names (e.g. Image, Picture, ImageComponent, Images, ResponsiveImages), but they all imply that there is something other than art direction happening.

@vpicone
Copy link
Contributor Author

vpicone commented Jul 31, 2019

@alisonjoseph yeah that's correct. I updated the documentation to explain more clearly:

You can place up to three images inside of the ArtDirection component. The first will be used for mobile, the second for tablet, and the third for desktop. If only two images are provided, the second image will be used for both tablet and desktop.

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

LGTM! I wish that the name ArtDirection made it more clear what it did but I cant think of anything better, I agree that it's better than the other more vague name examples you gave, @vpicone

@vpicone vpicone requested a review from emyarod August 1, 2019 16:36
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

thought I left an approval instead of a comment! I'm fine with the art direction name since it's the term for the problem we are solving here

@vpicone vpicone merged commit 78171d5 into carbon-design-system:master Aug 1, 2019
@vpicone vpicone deleted the ArtDirection branch August 1, 2019 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants