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

Image component for markdown components #427

Merged
merged 11 commits into from
Jul 14, 2020
Merged

Conversation

caylahamann
Copy link
Contributor

Description

This PR creates an image component for markdown components. It provides the same functionality as ![] syntax, but the hope is that we can expand on this to standardize our images"

Reviewer Notes

I replaced one component in one guide to demonstrate the difference. If you look at "Create a 'Hello, World!' application guide underneath the Build Apps section.

Related Issue(s) / Ticket(s)

Closes #328

@zstix zstix changed the base branch from master to main July 10, 2020 20:51
Comment on lines 4 to 5
const Image = ({ src, alt, width }) => {
return <img src={src} alt={alt} width={width} />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const Image = ({ src, alt, width }) => {
return <img src={src} alt={alt} width={width} />;
const Image = (props) => {
return <img {...props} />;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when I try to do this, i get a linting error because the linter wants there to be an alt prop, so I think i'm just going to keep it in the structure that I currently have it in

Copy link
Contributor

Choose a reason for hiding this comment

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

@caylahamann You can get around the lint error and apply @zstix's suggestion by destructuring the alt prop and spreading the rest of them like this:

const Image ({ alt, ...props }) => (
  <img alt={alt} {...props} />
)


return <Img fluid={data.placeholderImage.childImageSharp.fluid} />;
Image.defaultProps = {
width: 1200,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make more sense to make this width auto by default? Seems weird we'd want to specify a hard width for all images rather than let theme be their natural size until someone configures it otherwise. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my understanding of the ticket was that we wanted to standardize the size of the images (?) @mmfred @djsauble what are your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great point 😄 . I'll be curious to see what the others say .

Copy link
Contributor

Choose a reason for hiding this comment

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

Images that are too large in the vertical dimension make it hard to follow the flow of the page. For example:

Screen Shot 2020-07-13 at 10 52 14 AM

In contrast, images that take the full width of the page can be fine as long as they're not too tall. For example:

Screen Shot 2020-07-13 at 10 56 14 AM

My suggest is to 1) set a max-height of 400px for images, 2) let the width be up to 100% while respecting the aspect ratio of the image, 3) left align images so they follow the shape of the text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! thank you for those suggestions @djsauble

@caylahamann caylahamann added enhancement New feature or request work in progress This is work that is not yet ready for review labels Jul 14, 2020
Copy link
Contributor

@zstix zstix left a comment

Choose a reason for hiding this comment

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

😮

@caylahamann caylahamann merged commit 7bb5a31 into main Jul 14, 2020
@caylahamann caylahamann deleted the cayla/image-component branch July 14, 2020 21:06
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released work in progress This is work that is not yet ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stretch] Make a component to standardize image sizes for guides
5 participants