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(using-gatsby-image): refresh design, switch to emotion #9377

Merged
merged 48 commits into from
Nov 5, 2018

Conversation

fk
Copy link
Contributor

@fk fk commented Oct 25, 2018

Didn't have teh good internets for a couple of hours today, but a folder of unsplash.com images and just had touched on using-gatsby-image yesterday … so I decided to make some good use of the mostly offline dev time and revisit https://using-gatsby-image.gatsbyjs.org/.

Here's a video: https://www.dropbox.com/s/axq2zp8lna9y71y/using-gatsby-image.mov?dl=0

image

image

bildschirmfoto 2018-10-25 um 02 42 36

bildschirmfoto 2018-10-25 um 02 43 45

Todo:

  • Fix image credits (<img title>)
  • Optimize image query width/maxWidth
  • Refactor CSS to template literals…

/cc @gatsbyjs/website

Copy link
Contributor

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

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

This looks amazing, @fk!

A couple things noted inline that I'd like to see cleaned up for consistency and to drive a "Gatsby standard" of coding.

Also, a lot of these new images are huge — I'm not sure we want to put an additional 30MB of images into the repo for people to download. Maybe we can reduce the raw images down to whatever the maximum size is to avoid the impact on the overall repo size?

Thanks for putting this together!

/>
<Img
style={{ display: `inherit` }}
css={{
Copy link
Contributor

Choose a reason for hiding this comment

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

We had a discussion about styling that landed on using the styled component approach for Emotion, meaning we'll want to refactor this to be:

const GatsbyImage = styled(Img)`
	margin-bottom: ${rhythm(options.blockMarginBottom * 2)};
	/* ... */
`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, followed that and already did use styled for stuff I had to touch, but not everywhere else (yet). Happy to do so consistently!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought this was all new code.

I'll let @amberleyromo make the call about how much we should refactor. My vote would be to just knock it out while we're in here, but she's the final word 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.

@fk I would say if you feel it won't take too long and you feel good about it, go ahead and do it. Would be nice to have it as a good standalone example of the code style, since www will be switched over incrementally.

If not, don't stress, we can decide where it fits into priorities.

Copy link
Contributor Author

@fk fk Oct 26, 2018

Choose a reason for hiding this comment

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

@jlengstorf Not all new, just a little realign :D (also FWIW not all images are new, I tried to balance out old/new amount (add one/delete one), and plan on still removing a bunch more)

@amberleyromo I'll give it a shot because I also want to use styled a little more. Btw.…already got annoyed a couple times having to name things again ;)

[presets.Desktop]: {
borderWidth: 12,
},
"& img": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the styled component approach will make this easier, too:

const Component = styled('div')`
	background-color: red;

	img {
		border-radius: 2px;
	}
`;

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call that "easier", just different

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry — "easier" is in reference to the syntax being less complex, but agreed that it's just different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I find it harder, honestly, but it's just because I'm not as familiar with it.

Copy link
Contributor

@DSchau DSchau Oct 25, 2018

Choose a reason for hiding this comment

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

Harder than the css prop? It's the same style object in both places, just hoisted in one example and using a prop in the other, right?

Copy link
Contributor

@greglobinski greglobinski Oct 30, 2018

Choose a reason for hiding this comment

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

@fk First of all the page looks great ❤️

I support @jlengstorf with using emotion full syntax with tagged template literals #9377 (comment)

styled("div")`
...
`

There are a couple of ways to compose styles.

  1. The css way
const baseStyle = css`
  flex-grow: 1;
  font-size: 0.85rem;
  margin: 0;
  color: ${colors.gray.lightCopy};
  padding: ${rhythm(0.5)} 0 0;
`
const Description = styled("p")`
  ${baseStyle};
  color: red;
  1. Re-styling components way
const Description = styled("p")`
  flex-grow: 1;
  font-size: 0.85rem;
  margin: 0;
  color: ${colors.gray.lightCopy};
  padding: ${rhythm(0.5)} 0 0;
`

const DescriptionBig = styled(DescriptionBase)`
  font-size: 2rem;
`
  1. merging css classes with cx method
    https://emotion.sh/docs/cx

  2. And of course using dynamic styles
    https://emotion.sh/docs/composition#composing-dynamic-styles

I think that we should prefer the fourth and the second methods choose which ways we prefer.

/cc @amberleyromo @jlengstorf @DSchau

Copy link
Contributor

Choose a reason for hiding this comment

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

@fk's point is those are all much less sophisticated than what you can do when styles are data not strings you're concatenating together.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make styles as strings work but it feels like a big step back imo

Copy link
Contributor

Choose a reason for hiding this comment

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

@KyleAMathews Sure, it could be that I do not spot some use cases when the objects work better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are edge cases where styles work better, but overall moving to a JS abstraction over another language is a barrier for contributors who already know CSS.

My preference is to avoid logic in styles, and instead to have base styles that get extended where necessary:

const Button = styled('button')`
  background: red;
  border-radius: 0.5rem;
  color: white;
  font-size: 1.5rem;
`;

const BigButton = styled(Button)`
  font-size: 2.5rem;
`;

This makes it extremely clear that BigButton is an extension of Button, and doesn't require keeping how both JS and CSS work in your brain at the same time.

Conversely, the JS version looks less like styles and (for my brain) is significantly harder to parse whether it's logic for the UI or logic for the styles:

const Button = styled('button')(props => ({
  background: 'red',
  borderRadius: '0.5rem',
  color: 'white',
  fontSize: props.isBig ? '2.5rem' : '1.5rem',
}));

It's also less clear which button is being used in the second case, because the styles are now tied to the props and the result is hidden.

It's definitely going to come down to preference at the end of the day, but I feel pretty strongly that JS styles don't create enough benefit to justify the extra cognitive overhead.

@fk
Copy link
Contributor Author

fk commented Oct 25, 2018

Maybe we can reduce the raw images down to whatever the maximum size is to avoid the impact on the overall repo size?

👍

@pieh
Copy link
Contributor

pieh commented Oct 25, 2018

Also, a lot of these new images are huge — I'm not sure we want to put an additional 30MB of images into the repo for people to download. Maybe we can reduce the raw images down to whatever the maximum size is to avoid the impact on the overall repo size?

How about creating unsplash source plugin? Or even just hardcoding urls to download images from unsplash for this example site?

@amberleyromo
Copy link
Contributor

@pieh
Copy link
Contributor

pieh commented Oct 25, 2018

@pieh vacas5/gatsby-source-unsplash

It doesn't actually download images (so we can process them) - it has urls to remote unsplash cdn ( https://github.com/vacas5/gatsby-source-unsplash/blob/master/gatsby-node.js )

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

This looks great! As Jason said, images are probably going to be an issue--but otherwise this seems like a big improvement from a code & design perspective.

examples/using-gatsby-image/src/components/navigation.js Outdated Show resolved Hide resolved
@@ -0,0 +1,17 @@
const colors = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this a lot!

@KyleAMathews
Copy link
Contributor

We could write a little extension to source-airtable that uses createRemoteFileNode to pull down the actual image.

@KyleAMathews
Copy link
Contributor

Extension in the site's gatsby-node.js and onCreateNode

fk and others added 3 commits November 2, 2018 11:22
…at (#9605)

* feat: add a remote-images plugin so as to not introduce bloat

Note: this PR requires #9349 to be merged

* chore: source all images (besides one) from unsplash/remote data

* chore: bump all deps (to get remote image changes in
gatsby-source-filesystem)
- bring back image `title`/credits for homepage image, `floatingImage` and `fullWidthImage`, and `ImageGallery` images
- use function component for `TracedSVG`
- fix Gatsby logo position
- rn presets.Hd to presets.Xl
- add presets.Xxl
- declare units along setting presets.gutter and presets.offset values
- adjust `maxWidth` image query values
* refactor last pieces of CSS to use template literals
* „namespace“ media query constants in `presets` (`presets.mq`), lowercase `em
* rename `presets`’ `zIndex` to `elevation`
* fix `floatingImage` negative margin
* adjust modular scale ratio to major third, and stick to it when using `scale`
* simplify navigation active state styles
* small fix to the README of the local gatsby-source-remote-images plugin
* (almost) no more `px` and `!important`
* add link to image-processing.gatsbyjs.org
@fk fk changed the title [wip] feat(using-gatsby-image): refresh design, switch to emotion feat(using-gatsby-image): refresh design, switch to emotion Nov 5, 2018
@fk fk merged commit 2be8532 into master Nov 5, 2018
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…#9377)

* Initial commit :P

* Squash a few bytes

* Yo

* Don’t bind query name to image author

* Breakpoints

* wat

* Tidy z-index mess

* Use `styled`

* Doc, order, be explicit

* Use `styled`

* Simpler

* Moar `styled`, remove typography-theme-elk-glen

* Mv layouts/index -> components/layout

* Order

* Lint

* Escape from L.A.

* No named colors

* No PropTypes for now

* Don’t link away from example site

* Moar `styled`

* Order, z-index (again)

* Rm React.Fragment

* Fiddle around with image sizes

* Font size, use `blockMarginBottom`

* Images have no border-radius

* Allow setting title and background for `FloatingImage`

* Fix background colors, titles

* Limit `tracedSVG` gallery to subset of images

* rename folder
* rename query
* adjust query filter (`relativePath` -> `relativeDirectory`)

* Reduce image dimensions/file size, go with default quality where possible

* Shorten query filename regexes

* Rm two more images, add „-unsplash“ to all unsplash images

* Fix regex

* Use preset constants

* Drop object styles

* Duh

* Drop moar object styles

* Moar template literals

* feat(examples): add a remote-images plugin so as to not introduce bloat (gatsbyjs#9605)

* feat: add a remote-images plugin so as to not introduce bloat

Note: this PR requires gatsbyjs#9349 to be merged

* chore: source all images (besides one) from unsplash/remote data

* chore: bump all deps (to get remote image changes in
gatsby-source-filesystem)

* OCD

* Fix image `title`, tidy

- bring back image `title`/credits for homepage image, `floatingImage` and `fullWidthImage`, and `ImageGallery` images
- use function component for `TracedSVG`
- fix Gatsby logo position

* Don’t associate query title with image content

* Component name

* Function component

* Add mask-image to mobile navigation

* Fix layout (and coverImage) width for `Xxl` screens

- rn presets.Hd to presets.Xl
- add presets.Xxl
- declare units along setting presets.gutter and presets.offset values
- adjust `maxWidth` image query values

* Duh

* Fix logo link duh

* Finishing move 🤞

* refactor last pieces of CSS to use template literals
* „namespace“ media query constants in `presets` (`presets.mq`), lowercase `em
* rename `presets`’ `zIndex` to `elevation`
* fix `floatingImage` negative margin
* adjust modular scale ratio to major third, and stick to it when using `scale`
* simplify navigation active state styles
* small fix to the README of the local gatsby-source-remote-images plugin
* (almost) no more `px` and `!important`
* add link to image-processing.gatsbyjs.org
@fk fk deleted the topics/using-gatsby-image-design-refresh branch March 8, 2019 15:33
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.

7 participants