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

Make Placeholder component "element-query-like" responsive. #18745

Merged
merged 3 commits into from Dec 19, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Nov 26, 2019

This PR is courtesy of @jameslnewell who created the initial work in master...jameslnewell:element-queries. I added some additional commits but did not have the necessary access to James' repository, so I pushed it to origin instead. Let there be no doubt, props: @jameslnewell.

What this PR does, is make the <Placeholder /> component responsive using a custom form of element queries.

Before:

Screenshot 2019-11-26 at 11 35 55

After:

after

The above two screenshots also show why normal responsiveness is not sufficient. The Placeholder component is UI that needs to adapt to various nesting scenarios and dimensions.

To make that happen, I created 3 classes:

  • Larger than 320px applies size-lg (size large) to the Placeholder component.
  • Between 320px and 160px applies size-md (size medium)
  • Below 160px, applies size-sm (size small)

For the time being, this solves a problem with the Placeholder component. However we have many other UI elements that can benefit from responsive improvements, so I would love to hear your thoughts on how we can refactor these utility classes to be perhaps more automated or generic or at the very least easy to apply beyond just this Placeholder component.

Design-wise I leveraged those utility classes to reduce the UI as the component grows smaller, including hiding descriptions and icons when there isn't space. I simply removed the Upload icon — it does not feel like it adds value but in fact adds visual clutter instead. Additionally, I left-aligned the text, Placeholder labels, and buttons. This most literally anchors the content in a way that really helps reduce the visual appearance when more than one placeholder is visible on the page at the same time, which is going to be increasingly likely as templates grow in strength!

Your thoughts are very welcome.

@@ -44,6 +44,7 @@
"mousetrap": "^1.6.2",
"re-resizable": "^6.0.0",
"react-dates": "^17.1.1",
"react-resize-aware": "^3.0.0-beta.7",

This comment has been minimized.

Copy link
@jameslnewell

jameslnewell Nov 26, 2019

Contributor

Relying on a beta probably isn't ideal, but their method of measuring DOM changes is superior to other packages like react-use-dimensions because it will trigger an update even when content changes outside of a render() cycle. Maybe react-use-dimensions is enough for this use case 🤷‍♂

Edit: With a polyfill, https://www.npmjs.com/package/@rehooks/component-size could work

This comment has been minimized.

Copy link
@tellthemachines

tellthemachines Dec 13, 2019

Contributor

Apart from window resizing situations (and we know only devs and designers do that, right? 😂 ) is there any real need to update outside of render? If not, react-use-dimensions seems perfect for what we need. I'd be reluctant to go the component-size route because that polyfill for resize-observer is massive 🙀

This comment has been minimized.

Copy link
@jasmussen

jasmussen Dec 13, 2019

Author Contributor

Apart from window resizing situations (and we know only devs and designers do that, right? 😂 ) is there any real need to update outside of render?

The update-on-resize is nice, but definitely something we can live without. Worst case, seems like we could apply a transition: width 0.2s ease; and at least have the resizing happen in a way that looks less jarring than an instant snap. 👍 👍

This comment has been minimized.

Copy link
@jameslnewell

jameslnewell Dec 13, 2019

Contributor

Its not just window resizing that might cause the dimensions to change. If the width of another element grows or shrinks it might change dimensions of this element without this component having been re-rendered. The use of the iframe is a neat way of detecting these layout changes (roughly, when the iframe dimensions change it fires the resize event on the iframe window).

Given the multi-column layouts in the above screenshots, I think this case is probably something that we need to account for?

The polyfill is definitely huge! Can't wait for a native implementation that works in all browsers! Maybe we can offer the library author some assistance to get it out of beta.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Dec 13, 2019

Author Contributor

Its not just window resizing that might cause the dimensions to change.

This is a good point I had missed. Here's a use-case:

responsive

This comment has been minimized.

Copy link
@jameslnewell

jameslnewell Dec 13, 2019

Contributor

I'll reach out to the developer and see how we might be able to help.

Edit: Or we could refactor to use v2

This comment has been minimized.

Copy link
@jameslnewell

jameslnewell Dec 15, 2019

Contributor

@jasmussen The author has released a new version for us! Try bumping the version to ^3.0.0.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 26, 2019

As you can see from the deepest nesting level for the image, even more work is necessary, but this is a good start that improves the situation. Likely for the additional work, it might be worth looking at that in light of the new efforts ongoing in #18667.

@@ -1,15 +1,15 @@
.components-placeholder {
position: relative;

This comment has been minimized.

Copy link
@jameslnewell

jameslnewell Nov 26, 2019

Contributor

This is required to position and size the resizeListener correctly.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 17, 2019

Contributor

That comment might good in code.

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Nov 26, 2019

Tried to see if some other web builders use similar placeholders. Wix, Weebly, and Squarespace pre-fill in placeholders with demo content or fake images, instead of opting for showing buttons like we do, so they aren't much help.

Does any other CMS use block placeholders like we do?

Notion works similarly to us, but has similar responsive problems on the desktop app. Anyone have the mobile app and can check how placeholders work there?

@chaselangr

This comment has been minimized.

Copy link

chaselangr commented Nov 26, 2019

Mailchimp has a block placeholder that is pretty similar to the WordPress interface. Mailchimp uses a single button for replacing an image rather than options for URL, media library, and direct upload. Personally, I think it would work well if the buttons had the same width/max-width and the text was centered on the button. I think this might add some continuity and possibly make them more accessible for mobile users.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 27, 2019

LivingDocs uses a square cross which is very classic Desktop Publishing-like, but with no buttons.

I think the element query nature, of which this PR is just a very first early attempt, is the key way to improve this, as it allows us to optimize for both large and small contexts.

The thing is, the placeholder or "setup state" for the block is, to many blocks, the best UI to show next steps, when there's space. Take the Cover block, it feels like it's using the color swatches in a really nice way, letting you quickly pick a color and get moving!

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Nov 29, 2019

I like the shift to left-aligned text. It feels slightly cleaner and easier to read to me.

At the smallest breakpoint, I wonder if the placeholder could be reduced to just a single button that says "Add image…" or "Add media…" or something like that, with no placeholder title above. Would that reduce accessibility?

Also, should there be an attempt to make the full placeholder description available at the smaller breakpoints somehow? Perhaps an info icon that opens a pop-up containing the description?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 2, 2019

At the smallest breakpoint, I wonder if the placeholder could be reduced to just a single button that says "Add image…" or "Add media…" or something like that, with no placeholder title above. Would that reduce accessibility?

It seems like it should be fine. The block name, even if a placeholder, will be announced as such, and when you tab into the "Add..." button it's behavior should be clear. Elements that are display: none; will not be announced or focused. Here's the full placeholder:

focus

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 3, 2019

What's missing here?

As the conversation continues in #18800, it could make sense to merge this as a first step on the path.

@jasmussen jasmussen force-pushed the add/element-queries branch 2 times, most recently from 565ffa8 to 0c2cac0 Dec 3, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 3, 2019

Rebased and gave it a good squash.

@jasmussen jasmussen force-pushed the add/element-queries branch 2 times, most recently from 8b41c29 to 734d155 Dec 16, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 16, 2019

Thank you @jameslnewell ! I bumped in f26db0c#diff-fcafbf9322186d9a2998fd8c67903791R49. I also gave this a good rebase and a squash to simplify a final review. Let me know if things look as they should!

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 16, 2019

Testing this, I would love to see a variation in the 3 button's but for now, with our existing patterns, I think it's ok to ship.

image

@jasmussen jasmussen requested a review from Soean as a code owner Dec 17, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 17, 2019

Polished up the placeholders:

Screenshot 2019-12-17 at 11 14 14

Screenshot 2019-12-17 at 11 14 17

Each of these scale reasonably well to the smaller breakpoints as well through wrapping and the generic rules. They could use additional polish, but I'd rather prefer to keep this PR as small as can be, and we know this needs additional love soon.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 17, 2019

Functionally, everything looks better than master to me! 👍

@@ -9,7 +9,7 @@
background: none;
transition: box-shadow 0.1s linear;
@include reduce-motion("transition");
height: 36px;
height: $icon-button-size;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 17, 2019

Contributor

We should rename that variable (it's not just about icon) but we should do that separately.

Copy link
Contributor

youknowriad left a comment

Left some small comments, but LGTM overall.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 17, 2019

Thank you Riad. I will address those and we can take a final look!

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 18, 2019

@youknowriad I see a LGTM from you, but I forgot this comment from @tellthemachines:

The polyfill is definitely huge! Can't wait for a native implementation that works in all browsers! > Maybe we can offer the library author some assistance to get it out of beta.

Think it's okay to merge if the checks pass?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 18, 2019

Oh wait I misunderstood the full context. The polyfill that would be an alternative to what we're using here, is huge. Nevermind me. I'll try a rebase.

@jasmussen jasmussen force-pushed the add/element-queries branch 2 times, most recently from 78136c4 to 7904999 Dec 18, 2019
@jasmussen jasmussen force-pushed the add/element-queries branch from 7904999 to e484608 Dec 18, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 18, 2019

It looks like the test failure is legitimate:

✕ should not display an instructions element when smaller than 320px (4ms)

It's probably because in the initial version of this PR, the resize-aware was used to simply not output the description node below the 320px breakpoint, but I moved that to be CSS classes with display: none;. @jameslnewell do you think you could help me fix up those tests? I found some copy pasta that I might be able to get to work, but my Jest skills are pretty limited.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 19, 2019

@jasmussen I'll look at the tests.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 19, 2019

I removed the "instructions" hiding unit test because it assumed the hiding was done in JS but it's done in CSS and it's harder to test that way.

1 similar comment
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 19, 2019

I removed the "instructions" hiding unit test because it assumed the hiding was done in JS but it's done in CSS and it's harder to test that way.

Copy link
Contributor

youknowriad left a comment

Nice PR

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 19, 2019

Thank you Riad, I'll merge when the checks pass. Props @jameslnewell.

@jasmussen jasmussen merged commit 8b1f05b into master Dec 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the add/element-queries branch Dec 19, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@vicolaspetru

This comment has been minimized.

Copy link

vicolaspetru commented Jan 14, 2020

After update to 7.2.0, placeholder instructions is hidden, but why? I saw the class .is-small being added to placeholder component.
image

The token fields looks broken:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.