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

Iterations on the welcome guide #21847

Merged
merged 37 commits into from
May 18, 2020
Merged

Conversation

enriquesanchez
Copy link
Contributor

@enriquesanchez enriquesanchez commented Apr 23, 2020

Closes #19209, still WIP.

This PR updates the Welcome Guide following the iterations proposed on #19209. It updates the layout.

This PR can be tested on gutenberg.run/21847

@github-actions
Copy link

github-actions bot commented Apr 23, 2020

Size Change: +264 kB (23%) 🚨

Total Size: 1.11 MB

Filename Size Change
build/annotations/index.js 3.62 kB -2 B (0%)
build/api-fetch/index.js 3.39 kB -687 B (20%) 🎉
build/autop/index.js 2.83 kB +1 B
build/block-directory/index.js 6.93 kB +697 B (10%) ⚠️
build/block-directory/style-rtl.css 790 B +30 B (3%)
build/block-directory/style.css 791 B +30 B (3%)
build/block-editor/index.js 105 kB -548 B (0%)
build/block-editor/style-rtl.css 10.8 kB +664 B (6%) 🔍
build/block-editor/style.css 10.8 kB +667 B (6%) 🔍
build/block-library/editor-rtl.css 7.19 kB +56 B (0%)
build/block-library/editor.css 7.19 kB +61 B (0%)
build/block-library/index.js 118 kB +5.86 kB (4%)
build/block-library/style-rtl.css 7.48 kB +293 B (3%)
build/block-library/style.css 7.48 kB +291 B (3%)
build/block-serialization-default-parser/index.js 1.88 kB +3 B (0%)
build/blocks/index.js 48.1 kB -9.57 kB (19%) 👏
build/components/index.js 182 kB -16 kB (8%)
build/components/style-rtl.css 17.1 kB +164 B (0%)
build/components/style.css 17.1 kB +166 B (0%)
build/compose/index.js 6.67 kB +13 B (0%)
build/core-data/index.js 11.4 kB -6 B (0%)
build/date/index.js 5.47 kB -1 B
build/dom/index.js 3.11 kB +10 B (0%)
build/edit-navigation/index.js 6.6 kB +3.06 kB (46%) 🚨
build/edit-navigation/style-rtl.css 857 B +372 B (43%) 🚨
build/edit-navigation/style.css 856 B +371 B (43%) 🚨
build/edit-post/index.js 302 kB +274 kB (90%) 🆘
build/edit-post/style-rtl.css 12.2 kB -281 B (2%)
build/edit-post/style.css 12.2 kB -281 B (2%)
build/edit-site/index.js 12 kB +1.23 kB (10%) ⚠️
build/edit-site/style-rtl.css 5.22 kB -27 B (0%)
build/edit-site/style.css 5.22 kB -25 B (0%)
build/edit-widgets/index.js 7.87 kB -468 B (5%)
build/edit-widgets/style-rtl.css 4.69 kB -310 B (6%)
build/edit-widgets/style.css 4.69 kB -313 B (6%)
build/editor/editor-styles-rtl.css 425 B -3 B (0%)
build/editor/editor-styles.css 428 B -3 B (0%)
build/editor/index.js 44.3 kB +983 B (2%)
build/editor/style-rtl.css 5.07 kB +1.8 kB (35%) 🚨
build/editor/style.css 5.08 kB +1.81 kB (35%) 🚨
build/element/index.js 4.65 kB -1 B
build/format-library/index.js 7.64 kB +319 B (4%)
build/hooks/index.js 2.13 kB -2 B (0%)
build/keyboard-shortcuts/index.js 2.51 kB -4 B (0%)
build/keycodes/index.js 1.94 kB -2 B (0%)
build/list-reusable-blocks/index.js 3.13 kB +5 B (0%)
build/media-utils/index.js 5.29 kB +2 B (0%)
build/notices/index.js 1.79 kB -4 B (0%)
build/plugins/index.js 2.56 kB -110 B (4%)
build/primitives/index.js 1.5 kB +8 B (0%)
build/redux-routine/index.js 2.85 kB +10 B (0%)
build/rich-text/index.js 14.8 kB -1 B
build/server-side-render/index.js 2.68 kB +2 B (0%)
build/url/index.js 4.02 kB +3 B (0%)
build/viewport/index.js 1.84 kB +1 B
build/warning/index.js 1.14 kB -1 B
build/wordcount/index.js 1.17 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/blob/index.js 620 B 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.43 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/priority-queue/index.js 789 B 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B

compressed-size-action

@enriquesanchez
Copy link
Contributor Author

enriquesanchez commented Apr 24, 2020

Here's the current state of this PR:

2020-04-24 15-08-06 2020-04-24 15_09_01

There are some issues that I'm still working through:

  1. The proposed mockups display the pagination dots just below the image. This apparently is not that simple to do, since it'll require a bit of a refactor of the component, and doing so could cause those pagination dots to disappear for any third-party developers who are using the component. @noisysocks is currently helping me digure out the viability of such change. Update: this is no longer an issue.

  2. If we were to keep the pagination dots at the bottom in the same row as the Prev/Next buttons, there's not enough space once you reach the last slide and the 'Get Started' button appears. I've already made the dots smaller and placed them closer together, but the available space is still not enough. And again, this will probably get worse once we introduce internationalization into the mix. Update: this is no longer an issue.

  3. Moving from slide to slide, you'll notice a change in the modal's height. This is caused because the amount of text varies from slide to slide. Realistically, we should be OK with the fact that we don't have control over this, particularly if we consider internationalization and other uses of the component, and should look at alternatives.

@enriquesanchez
Copy link
Contributor Author

@mapk @pablohoneyhoney @mtias I'd appreciate it if you could please give this PR a test and let me know what you think about the issues I mentioned in the comment above.

@noisysocks
Copy link
Member

  1. The proposed mockups display the pagination dots just below the image. This apparently is not that simple to do, since it'll require a bit of a refactor of the component, and doing so could cause those pagination dots to disappear for any third-party developers who are using the component. @noisysocks is currently helping me digure out the viability of such change.

Hey @enriquesanchez, I created 9e6d4e8 which deprecates the Guide component's children prop in lieu of a less flexible but more structured pages prop. This lets us change Guide to render the pagination dots in between the image and the content.

My commit just modifies the markup and does not implement any of the necessary CSS changes. (I'll leave that up to you! 😛) If you'd like to add my commit to your branch and play with it, you can run:

git checkout try/evolve-the-welcome-guide
git cherry-pick 9e6d4e87033867a09f6561e71058267384b5cbce

@enriquesanchez
Copy link
Contributor Author

Thank you @noisysocks!

@enriquesanchez
Copy link
Contributor Author

Here's an update after the lastest round of changes:

2020-04-27 15-53-53 2020-04-27 15_54_35

@enriquesanchez
Copy link
Contributor Author

@ballio2000 These look great!

I think we can move forward and generate the gif files.

@ballio2000
Copy link

@enriquesanchez @mtias Please find the optimized GIFs attached. They're all set to play once and stop. Let me know if you all need anything else.
-g2-intro-01
-g2-intro-02
-g2-intro-03
-g2-intro-04

@enriquesanchez
Copy link
Contributor Author

I've been trying to implement the prefers-reduced-motion query for these animated gifs, but because the static SVGs are set as inline images...

export const EditorImage = ( props ) => (
  <img
    alt=""
    src="data:image/svg+xml,%3Csvg fill='none' height='240' viewBox='0 0 312 240' width='312' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='m0 0h312v240h-312z' fill='%2300a0d2'/%3E%3Crect fill='%23fff' height='108' rx='2' width='216' x='48' y='80'/%3E%3Cg stroke='%23000' stroke-width='1.5'%3E%3Cpath d='m158.917 142v-15.111'/%3E%3Cpath d='m154.472 142v-15.111'/%3E%3Cpath d='m162.333 126.75h-8.889'/%3E%3Cpath d='m153.139 130.889v4.071c-1.928-.353-3.389-2.041-3.389-4.071s1.461-3.718 3.389-4.071z' fill='%23000'/%3E%3C/g%3E%3Crect fill='%23fff' height='21' rx='1.5' stroke='%231e1e1e' width='117' x='48.5' y='53.5'/%3E%3Cpath d='m70.592 53v22' stroke='%231e1e1e'/%3E%3Cpath d='m144.432 53v22' stroke='%231e1e1e'/%3E%3Crect fill='%23333' height='8' rx='1' width='9' x='55' y='60'/%3E%3Cpath d='m150 63h2v2h-2z' fill='%23333'/%3E%3Cpath d='m154 63h2v2h-2z' fill='%23333'/%3E%3Cpath d='m158 63h2v2h-2z' fill='%23333'/%3E%3C/svg%3E"
    { ...props }
  />
);

... we can't make use of the prefers-reduced-motion media query in the CSS.

I found this other technique that uses the <picture> element and it's very cool. The idea is to do something like...

<picture>
  <source srcset="no-motion.jpg" media="(prefers-reduced-motion: reduce)"></source> 
  <img srcset="motion.gif" />
</picture>

The above includes both media elements, and the <source> element has the prefers-reduced-motion: reduce media query as an attribute. This led me to try...

export const CanvasImage = ( props ) => (
  <picture>
     <source 
      srcset="data:image/svg+xml,%3Csvg fill='none' height='240' viewBox='0 0 312 240' width='312' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='m0 0h312v240h-312z' fill='%2300a0d2'/%3E%3Cpath d='m48 32c0-1.1046.8954-2 2-2h212c1.105 0 2 .8954 2 2v208h-216z' fill='%23fff'/%3E%3Cpath d='m60 38h191.455v34h-191.455z' fill='%23ddd'/%3E%3Cpath d='m151 49v11l5-4.125 5 4.125v-11h-5z' fill='%23000' stroke='%23000' stroke-width='1.5'/%3E%3Cpath d='m48 80h216v74h-216z' fill='%23e3e3e3'/%3E%3Crect height='16.5' rx='1.53571' stroke='%23000' stroke-width='1.5' width='16.5' x='147.75' y='108.75'/%3E%3Cpath d='m154 120v-6l5 3z' fill='%23000'/%3E%3Cpath d='m60 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cpath d='m159.982 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cg stroke='%23000' stroke-width='1.5'%3E%3Crect height='16.5' rx='1.25' width='16.5' x='97.75' y='179.75'/%3E%3Cpath d='m98 192 4.571-3.333 3.429 2.222 4-3.889 4 3.889' stroke-linejoin='round'/%3E%3Cpath d='m208.917 196v-15.111'/%3E%3Cpath d='m204.472 196v-15.111'/%3E%3Cpath d='m212.333 180.75h-8.889'/%3E%3Cpath d='m203.139 184.889v4.071c-1.928-.353-3.389-2.041-3.389-4.071s1.461-3.718 3.389-4.071z' fill='%23000'/%3E%3C/g%3E%3Cpath d='m60 220h191v20h-191z' fill='%23ddd'/%3E%3C/svg%3E"
      media="(prefers-reduced-motion: reduce)">
     { ...props }
    </source>
    <img
      alt=""
      srcset="my-animated.gif"
      { ...props }
    />
  </picture> 
);

... but that won't work. As far as I can tell, this is not going to work because <source> does not accept data URIs.

I was able to get this technique to work on a Codepen, but only if I use a URL of the SVG. It's actually really cool:

2020-05-08 17-30-44 2020-05-08 17_36_32


@noisysocks What do you think of this technique? Would we be able to use the URL of the SVG instead? If so, where would we store the images? We'd also need a place to store the animated GIFs.

@mapk
Copy link
Contributor

mapk commented May 9, 2020

cc @jasmussen and @kjellr for some prefer-reduced-motion help here as well.

@jasmussen
Copy link
Contributor

To include the prefers-reduced-motion mix-in you can simply do this:

	transition: 100ms transform ease;
	@include reduce-motion("transition");

☝️ that's just a code sample, the mixin is the key bit. Include that after the animation you add, and it should work. If it is to reduce an animation instead, include that:

	@include reduce-motion("animation");

@enriquesanchez
Copy link
Contributor Author

enriquesanchez commented May 12, 2020

Thanks for the feedback @jasmussen!

I'm still a bit confused, how is the mixin supposed to swap the image file? We have an animated GIF and want to change it for a static SVG. The tricky part is that these images are not declared as CSS background properties, they are declared inline. This is why I was looking at using the motion query in the <picture> element.

@jasmussen
Copy link
Contributor

Oh my mistake, the mixin won't help you there. Apologies.

You'll want to go straight to the prefers-reduced-motion query and write conditional css that hides the gif and shows the svg inside the query. There's some code here:

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion

Let me know if that helps. I'm afk for the rest of the day but if you don't get it working you can leave a note and I'll whip something up tomorrow.

@enriquesanchez
Copy link
Contributor Author

You'll want to go straight to the prefers-reduced-motion query and write conditional css that hides the gif and shows the svg inside the query.

Thank you @jasmussen, that's a great idea.


@noisysocks I don't suppose we can do something like:

export const CanvasImage = ( props ) => (
	<img
		class="edit-post-welcome-guide__image__01"
		alt=""
		src="https://enriquesanchez.mx/illustration.gif"
		{ ...props }
	/>
	<img
		class="edit-post-welcome-guide__image__01alt"
		alt=""
		src="data:image/svg+xml,%3Csvg fill='none' height='240' viewBox='0 0 312 240' width='312' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='m0 0h312v240h-312z' fill='%2300a0d2'/%3E%3Cpath d='m48 32c0-1.1046.8954-2 2-2h212c1.105 0 2 .8954 2 2v208h-216z' fill='%23fff'/%3E%3Cpath d='m60 38h191.455v34h-191.455z' fill='%23ddd'/%3E%3Cpath d='m151 49v11l5-4.125 5 4.125v-11h-5z' fill='%23000' stroke='%23000' stroke-width='1.5'/%3E%3Cpath d='m48 80h216v74h-216z' fill='%23e3e3e3'/%3E%3Crect height='16.5' rx='1.53571' stroke='%23000' stroke-width='1.5' width='16.5' x='147.75' y='108.75'/%3E%3Cpath d='m154 120v-6l5 3z' fill='%23000'/%3E%3Cpath d='m60 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cpath d='m159.982 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cg stroke='%23000' stroke-width='1.5'%3E%3Crect height='16.5' rx='1.25' width='16.5' x='97.75' y='179.75'/%3E%3Cpath d='m98 192 4.571-3.333 3.429 2.222 4-3.889 4 3.889' stroke-linejoin='round'/%3E%3Cpath d='m208.917 196v-15.111'/%3E%3Cpath d='m204.472 196v-15.111'/%3E%3Cpath d='m212.333 180.75h-8.889'/%3E%3Cpath d='m203.139 184.889v4.071c-1.928-.353-3.389-2.041-3.389-4.071s1.461-3.718 3.389-4.071z' fill='%23000'/%3E%3C/g%3E%3Cpath d='m60 220h191v20h-191z' fill='%23ddd'/%3E%3C/svg%3E"
		{ ...props }
	/>
);

...and then conditionally hide/show either of those images based on the motion query?

I'm assuming because I just tried it and got some BABEL failures.

Is there any other way we could accomplish this?

@noisysocks
Copy link
Member

Your snippet looks good except that you need to wrap the two images with <>...</> and use className instead of class.

export const CanvasImage = ( props ) => (
	<>
		<img
			className="edit-post-welcome-guide__image__01"
			alt=""
			src="https://enriquesanchez.mx/illustration.gif"
			{ ...props }
		/>
		<img
			className="edit-post-welcome-guide__image__01alt"
			alt=""
			src="data:image/svg+xml,%3Csvg fill='none' height='240' viewBox='0 0 312 240' width='312' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='m0 0h312v240h-312z' fill='%2300a0d2'/%3E%3Cpath d='m48 32c0-1.1046.8954-2 2-2h212c1.105 0 2 .8954 2 2v208h-216z' fill='%23fff'/%3E%3Cpath d='m60 38h191.455v34h-191.455z' fill='%23ddd'/%3E%3Cpath d='m151 49v11l5-4.125 5 4.125v-11h-5z' fill='%23000' stroke='%23000' stroke-width='1.5'/%3E%3Cpath d='m48 80h216v74h-216z' fill='%23e3e3e3'/%3E%3Crect height='16.5' rx='1.53571' stroke='%23000' stroke-width='1.5' width='16.5' x='147.75' y='108.75'/%3E%3Cpath d='m154 120v-6l5 3z' fill='%23000'/%3E%3Cpath d='m60 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cpath d='m159.982 163h91.4727v49h-91.4727z' fill='%23ddd'/%3E%3Cg stroke='%23000' stroke-width='1.5'%3E%3Crect height='16.5' rx='1.25' width='16.5' x='97.75' y='179.75'/%3E%3Cpath d='m98 192 4.571-3.333 3.429 2.222 4-3.889 4 3.889' stroke-linejoin='round'/%3E%3Cpath d='m208.917 196v-15.111'/%3E%3Cpath d='m204.472 196v-15.111'/%3E%3Cpath d='m212.333 180.75h-8.889'/%3E%3Cpath d='m203.139 184.889v4.071c-1.928-.353-3.389-2.041-3.389-4.071s1.461-3.718 3.389-4.071z' fill='%23000'/%3E%3C/g%3E%3Cpath d='m60 220h191v20h-191z' fill='%23ddd'/%3E%3C/svg%3E"
			{ ...props }
		/>
	</>
);

We shouldn't use https://enriquesanchez.mx/illustration.gif as the src because it will mean that WP Admin is reliant on a third party server. Instead, let's generate a data: URL for the GIF.

I suspect that all of these inline data: images are starting to make the JS bundle size too large. We should follow up on this work with a separate PR which moves the images out of data: URLs and into e.g. /wp-admin/images.

@enriquesanchez
Copy link
Contributor Author

Thanks for the feedback @noisysocks!

We shouldn't use https://enriquesanchez.mx/illustration.gif as the src because it will mean that WP Admin is reliant on a third party server.

Correct, I was just using that external URL temporarily to test, will make sure I update it.

I suspect that all of these inline data: images are starting to make the JS bundle size too large. We should follow up on this work with a separate PR which moves the images out of data: URLs and into e.g. /wp-admin/images.

Right, I was assuming I'll put the animated GIF files in /wp-admin/images, should I also do that for the SVGs then?

@enriquesanchez
Copy link
Contributor Author

I added the animated GIFs as data URI and all seems to work 👍

This is ready to be tested. If you're on a Mac, you can test the prefers-reduced–motion setting by going to System Preferences, select the Accessibility category, select the Display tab, and enable the Reduce Motion option.

On Windows 10, go to Settings, select Ease of Access, select Display, select Show animations in Windows.

When no preference has been set, you should see the animated GIFs play once (no loop). If you set your display to reduced motion, then you should not see the animated GIF's and a static illustration should appear instead.

@noisysocks
Copy link
Member

Right, I was assuming I'll put the animated GIF files in /wp-admin/images, should I also do that for the SVGs then?

Yes, I think moving every SVG and GIF image to /wp-admin/images makes sense. But don't have to do it in this PR 🙂

Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

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

I just tested this and it worked well from a design perspective. I haven't tried with reduced motion yet though.

welcome

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

The code LGTM!

Let's definitely follow-up with a PR to move those image files elsewhere instead of using data: URLs as they're quite large! 🙂

packages/components/src/guide/README.md Outdated Show resolved Hide resolved
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.

Iteration on the Welcome Guide
7 participants