Skip to content
This repository has been archived by the owner on Feb 16, 2021. It is now read-only.

feat(sqip): implement sqip && fix mobile layout #13

Closed
wants to merge 5 commits into from
Closed

Conversation

axe312ger
Copy link
Contributor

updates all dependencies and fully implements sqip previews

@axe312ger axe312ger requested a review from stefanjudis May 17, 2018 20:40
@axe312ger axe312ger changed the title feat(sqip): implement sqip feat(sqip): implement sqip && fix mobile layout May 18, 2018
@axe312ger
Copy link
Contributor Author

The layout was pretty much broken on mobile. I fixed it via css grid via 9 additions and 12 deletions :)

iPad Landscape iPad Portrait iPhone Portrait
localhost_8000_ ipad 1 localhost_8000_ ipad localhost_8000_ iphone 6_7_8

package.json Outdated
@@ -7,22 +7,27 @@
"url": "https://github.com/contentful-userland/gatsby-contentful-starter/issues"
},
"dependencies": {
"contentful-import": "^6.2.0",
"gatsby-link": "^1.6.34",
"axios": "^0.18.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this dependency will be gone with gatsbyjs/gatsby#5469

package.json Outdated
"gatsby-transformer-remark": "^1.7.32",
"inquirer": "^5.1.0",
"lodash": "^4.17.4"
"gatsby-plugin-sharp": "^1.6.44",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this dependency will be gone with gatsbyjs/gatsby#5469

@@ -7,22 +7,27 @@
"url": "https://github.com/contentful-userland/gatsby-contentful-starter/issues"
},
"dependencies": {
"contentful-import": "^6.2.0",
Copy link
Contributor

@stefanjudis stefanjudis May 19, 2018

Choose a reason for hiding this comment

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

@axe312ger Why did you drop the import? That's part of the setup flow? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was on accident 🙈
good catch 👀

Copy link

@loudmouth loudmouth May 21, 2018

Choose a reason for hiding this comment

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

Can we use contentful-cli instead of contentful-import now that the latter is deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

They should use the contentful-import directly here because it is used as node library

Choose a reason for hiding this comment

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

makes sense! thanks!

height: 100%;
width: auto;
margin: 0 auto;
height: 61.8vh;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this number? pi * 👍🏻 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I googled golden ratio in percentage, this came out and it looked nice. So thats why I did choose this value 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha... Can you then add this as a comment? :D

Copy link
Contributor

@stefanjudis stefanjudis left a comment

Choose a reason for hiding this comment

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

I think this is great but bringing this in additionally I feel like we should have a place in the docs describing what is included in the starter? There's going more and more magic in which can be quite confusing. :)

What do you think?

@axe312ger
Copy link
Contributor Author

Alright, will add some docs :)

@axe312ger
Copy link
Contributor Author

Docs added, comment added, rebased.

Ready for review :)

@iammatthias
Copy link

Any update on this?

@Bloomca
Copy link

Bloomca commented Jul 9, 2018

@axe312ger I just tried your changes, and it looks very cool!

One problem, though, is starting time: now, instead of 3 seconds it is 65.

After this change:
image

Before this change:
image

Can we make this part (with sqip) optional? What do you think?

Thanks!

@axe312ger
Copy link
Contributor Author

Hey there,

optional could be nasty since it requires different config for the gatsby-image element.

Also, yes it takes more time to run in the first time but subsequent runs will use the already generated picture.

63 seconds is really slow, what kind of machine did you use?

Best,
Benedikt

@Bloomca
Copy link

Bloomca commented Jul 9, 2018

Ah, I see. I have MBP 2017, 3,1 GHz Intel Core i7 and 16GB DDR3, so it should be good.

I've just reinstalled all packages and it is 3 seconds again! Super weird, since I think I did before as well, but if it is for you the same (no changes in time) then I think all good.

@axe312ger
Copy link
Contributor Author

I am wondering if this gives first time Contentful/Gatsby users the impression it is slow.

Maybe this isn't that great idea and we should maybe switch to traced SVG output since this is supported now in Gatsby in combination with Contentful.

What do you think?

@Bloomca
Copy link

Bloomca commented Jul 9, 2018

Actually, I just tried it again, and it seems that fast rebuilding (3 seconds) is only if everything is in cache, so in case I do the following:

  1. clone the repo
  2. git checkout feat/sqip
  3. npm install
  4. npm run setup (with my credentials of empty newly created space)
  5. npm run dev

It takes ~50 seconds now (weird). Also, it does not really run for me (graphQL server works, but not usual http://localhost:8000), and also npm run build fails with this stack trace:

image

@axe312ger
Copy link
Contributor Author

Hmm thats very weird, looks like a Gatsby problem to me, for some reason it cant fine some files o.O

With the 50 seconds: Thats normal, primitive is a random algorithm so it might take some more or less time. But the settings here are maybe to crazy, it shouldn't take that long on your machine.

🤔

@stefanjudis
Copy link
Contributor

@axe312ger I have to say I agree with @Bloomca here, that 50s might give users the impression that this is the overall development experience.

Any chance to only get the CSS fixes in? 😊

@axe312ger
Copy link
Contributor Author

Alright. Here we go:

Mobile design: #25
Traced SVG: #26

@axe312ger axe312ger closed this Jul 23, 2018
@axe312ger axe312ger deleted the feat/sqip branch July 23, 2018 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants