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

Decrease scrim when in a modal overlay #27054

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Conversation

karmatosed
Copy link
Member

This is a little PR that certainly needs feedback. The problem it is designed to solve is specifically when you change a setting on preferences, it's hard to see what the change was. You can see that here:

2020-11-17 18 36 55

By increasing the transparency, you are able to more easily see the change in effect as it happens:

2020-11-17 22 06 02

This is useful when words are hard to describe what is going on. At times, simply seeing in the interface is a great help. I did explore having partial scrims and even none, they all resulted in a more negative experience or visual hitch.

Feedback

There are a few specific areas along with more general feedback that would be good to get input on whether there was a reason this was set to the opacity it was and by unsetting there is an impact.

@karmatosed karmatosed self-assigned this Nov 17, 2020
@karmatosed karmatosed added the Needs Design Feedback Needs general design feedback. label Nov 17, 2020
@brentswisher
Copy link
Contributor

My concern would be that this would introduce some accessibility issues. The purpose of the scrim is specifically to hide any changes in the background. Lightening the scrim means you are relying on someone seeing what is happening to understand the change - which kind of goes against the whole idea of a modal focusing the user? It looks like this was part of the reason @kjellr darkened it in #15974 at least.

If the problem is that the settings aren't clear, adjusting the modal content with better wording or even adding an example might be a better approach? Maybe you could try having a small example below the setting that changes with it?

@jasmussen
Copy link
Contributor

I don't mind it at all. The fact that the popop immediately takes attention feels focus grabbing enough (not to mention the literal focus grabbing).

However I believe we tried this in the past, and decided to keep the scrim the same as it is for the classic WordPress lightboxes such as the media library:

Screenshot 2020-11-19 at 08 06 07

I don't feel particularly strongly that the two need to stay in perfect lockstep, in fact longer term I'd prefer to replace the latter with the former. But sharing as context.

@karmatosed
Copy link
Member Author

karmatosed commented Nov 19, 2020

Thanks for the feedback so far everyone.

@brentswisher, I totally understand your concerns, however I think I can perhaps respond to a few of them with some context of how I got to this solution. I totally agree that iterating text and being as clear as possible should be the first step, there are cases where words just aren't enough. As explained, I have tried adding things like examples, but it ended up feeling like they were clickable or even increasing distraction. Now, this could be a case of needing to iterate what was showing, something I am open to exploring outside this PR.

This is where I would like to point out something, this suggestion isn't about removing the scrim, the idea is to adjust the effect. Initially it was brought in for a few reasons, there was inconsistency in there being white and dark scrims and there was also in some places none. What this does is reduce the strength. If the idea is to not see the background, I would argue when something is changing like a setting it could be more cognitive dissonance to see a slight shadow change than actual change. Of course, this could do with testing and feedback.

All of this said, if the feedback is to not progress here I am very open to that, I just wanted to be clear about why this exploration was being suggested. It really just is at this point an experiment so thank you both for joining me in early feedback.

@jasmussen your point about what we can or can't control in classic is making me pause here, if that is the case then I'm cautious about continuing and would like to review how the flow feels or how deep the fix would be. That feels like a next step for me!

@karmatosed
Copy link
Member Author

Here is what it looks like going between both screens, I can confirm there is no way to change this outside of a Trac patch.

2020-11-25 20 16 11

@jasmussen and anyone else, would love your feedback on how much of a visual hitch this is. My own feeling is if I'm honest I could go either way on this. It was an experiment and if opinion is to leave to one side for now I am quite fine with that.

@jasmussen
Copy link
Contributor

I like it, I'd be happy to try it. We could always position this as a "lighter" alternative to the bulkier media dialog modals.

@karmatosed
Copy link
Member Author

@jasmussen that's great. I do think because one is near-full and the other is in middle you almost get away without seeing it. I did follow quite a flow not many are going to also going between both of them.

Once the tests have passed I will get this merged and we can take from there. Thanks for input. As with anything we try in the editor this can of course be iterated and I like idea of seeing here before proposing wider.

@github-actions
Copy link

Size Change: 0 B

Total Size: 1.2 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 134 kB 0 B
build/block-editor/style-rtl.css 11.3 kB 0 B
build/block-editor/style.css 11.3 kB 0 B
build/block-library/editor-rtl.css 8.96 kB 0 B
build/block-library/editor.css 8.96 kB 0 B
build/block-library/index.js 148 kB 0 B
build/block-library/style-rtl.css 8.23 kB 0 B
build/block-library/style.css 8.23 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.95 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 828 B 0 B
build/data/index.js 8.8 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.42 kB 0 B
build/edit-post/style.css 6.4 kB 0 B
build/edit-site/index.js 24 kB 0 B
build/edit-site/style-rtl.css 3.92 kB 0 B
build/edit-site/style.css 3.92 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.87 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.81 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@ItsJonQ
Copy link

ItsJonQ commented Nov 26, 2020

I'm really not sure why these tests are failing 🤔 .

It looks like there's inconsistencies with the test results from the commits on master:
https://github.com/WordPress/gutenberg/commits/master

There should be no reason why this CSS change would break E2E Admin 1.
(Prior to @karmatosed 's rebase, E2E Admin 4 was broken, but E2E Admin 1 passed)

Based on this, I think it's okay to merge 👍

@karmatosed
Copy link
Member Author

Thanks for second opinion @ItsJonQ - let's merge and I am very happy to undo that if feedback arises.

@karmatosed karmatosed merged commit d7f0474 into master Nov 26, 2020
@karmatosed karmatosed deleted the try/modal-scrim-lighten branch November 26, 2020 16:05
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants