Update the modal design #40781
Update the modal design #40781
Conversation
| @@ -77,7 +82,7 @@ | |||
| left: 0; | |||
|
|
|||
| .components-modal__header-heading { | |||
| font-size: 1rem; | |||
| font-size: 1.2rem; | |||
There was a problem hiding this comment.
It would be nice for things like this to be systematised, I'm aware of the experimental Text component, but it's not widely used. Perhaps there's a better way?
There was a problem hiding this comment.
I'd personally love to see the Text and Heading components user more often.
I'm not sure if we need to first assess the type scale (and the rest of the styles), but otherwise I think we should slowly try to adopt them in other components, find out if they match our needs and make any necessary changes, before then rolling them out more widely across Gutenberg
|
Size Change: -116 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
I tested this on Gutenberg.run. Here's the welcome modal: While I'm sure the right decisions will be reached here regardless of my input, here are my opinions. I think the heading size change succeeds greatly. I'm not sure about removing the border. Though I like it at a glance, I don't love the lack of a defined edge to scrolling content: I think the blur is a win. The extra roundness on the corners seems alien. Great to see the work on this 👍 |
There was a problem hiding this comment.
A few thoughts:
- I think the blurring of the background is a big improvement in terms of modal focus, this is assuming there's no impact on performance (window resize, scrolling).
- I think the larger title is clearer.
- The missing under title border makes scrolling less clear. If this could appear once scrolled I think that could be a good compromise.
- The rounded corners -- are there any other examples of that size radius in Gutenberg? I like the look, but it seems like it departs from the current overall aesthetic.
I agree with this and ideally we could align the radii to the intent of the rest of the current, already refreshed system. |
|
Thanks for all the reviews :)
Is this documented anywhere, or is it just a case of always using a 2px radius? My rationale for updating it is to harmonise nested radii. I feel an awkward tension when there's no scale.
Yes this needs testing on a variety of setups. It feels fine on my 2019 MacBook Pro, but obviously that's a relatively capable machine.
Yeah that would be nice, would there be an impact on performance? |
imo I'd argue the opposite is true based on the images above. It doesn't need to be 2 in all applications, but certainly less rounded, retaining a bit more uniqueness and less trendiness. |
|
Just to clarify, it is a suggestion motivated by maths rather than trends :D
I'll revert for now though, this is probably something to look at more holistically. |
|
:D I think it's more to do with general design direction. |
|
Thank you @jameskoster for working on this! The modal definitely needs a bit of visual polish. Since this component is probably used in a few places across Gutenberg, we should make sure that the modal still looks good in all of these occurrences. I would also suggest using Storybook when working on a component, as it helps us to first make sure that a given component works well in isolation, before seeing how it integrated in the block editor. I also agree with the comments above re.:
Finally, could you add an entry to the package's CHANGELOG ? |
|
Here's the Modal updates in Storybook: storybook.mp4The background isn't blurred because I am targeting |
|
I don't think it makes sense to try to blur all the contents. Can it use |
|
It's an option, but browser support isn't as good. |
|
I suppose it doesn't hurt to consider the blur a progressive enhancement, the performance improvements are probably worth it. |
There was a problem hiding this comment.
Though I notice the border-on-scroll behaviour isn't being applied because the border is added to
.components-modal__contentwhich isn't present here. Maybe it would be better to add the border to .components-modal__header @ciampo?Would be good to fix that as it's a regression compared to what's in trunk.
That regression is actually caused by the fact that, specifically for the Query pattern selector block, some custom styles are preventing the modal's content wrapper from scrolling (and therefore never triggers the logic to show/hide the border):
gutenberg/packages/block-library/src/query/editor.scss
Lines 22 to 28 in 6d1496a
To fix the regression, that modal should be rewritten in a such a way that allows the .components-modal__content element to scroll.
Regarding whether the border should be applied to the "header" or the "content" wrapper, that's a matter of preference and, in my opinion, more of an implementation detail.
| @@ -113,26 +118,12 @@ | |||
| // Modal contents. | |||
| .components-modal__content { | |||
| flex: 1; | |||
| margin-top: $header-height; | |||
| margin-top: $header-height + $grid-unit-20; | |||
There was a problem hiding this comment.
Just flagging that this change may have introduced other layout regressions in places where it was assumed that the Modal's header was $header-height px tall.
One such example is the block pattern inserter/explorer modal, where in my testing I spotted a regression caused by the fact that the sidebar was positioned at $header-height px from the top of its container
In general, consumers of the Modal component should not assume implementation details like the height of its header — when that happens, a small change like this PR is enough to cause a regression (which is also what I was trying to flag in this other comment of mine ).
While a short-term fix would be to update the line of code linked above to use the updated Modal's header height, I would argue that the best way to fix this specific regression in the block pattern inserter / explorer is to rewrite the modal's internal layout in a way that avoids absolute positioning and hardcoded margins/positions.
There was a problem hiding this comment.
I would argue that the best way to fix this specific regression in the block pattern inserter / explorer is to rewrite the modal's internal layout avoiding absolute positioning and hardcoded distances.
I agree, the hardcoded height also causes issues where the modal title is long enough to wrap (uncommon but not impossible). Do you think we should do this as a part of this PR or elsewhere?
There was a problem hiding this comment.
Probably the best course of action is to:
- add a quick fix in this PR (should be as easy as changing from
top: $header-height;totop: $header-height + $grid-unit-20; - open a few follow-up PRs to:
- refactor the layout of the component as suggested before.
- try to refactor away from the hardcoded height as discussed in this other comment
+1 on merging and refining after the above quick fix. I don't think we need to bike shed on the amount of blur and hold this up. 👍 |
This is on my list to have as part of a broader call for testing for the FSE Outreach program to hopefully get some feedback for what it's worth (been following this PR). |
|
Pushed an update to fix the issue above:
Will merge once tests pass :) |
|
Just to summarize the follow-up work that we agreed during this review:
|
|
I really like the smooth look of the blurred background on the modal, but I just encountered a potential usability issue where there is an element outside of the modal that is contextually related to the content of the modal. For example, if you go to adjust an item in the List View to apply or remove a Lock, with the modal open, you can no longer see which (particular) block you have highlighted:
I'm curious if there are other cases where we have this sort of contextual issue — perhaps in the case of Lock, the use of a Modal isn't quite right, maybe it should be a Popover instead in order to better highlight the relationship to the currently selected item. (I'm happy to open up a separate issue if folks think the approach for the Lock modal should be adjusted rather than the styling of the Modal). |
|
As you alluded to, I think this could actually be a helpful guiding principle around when to (or not) use modals. I outlined in more detail here, but generally speaking if you need to observe both the modal contents and the rest of the UI together, then a modal is probably not going to be the best choice. That said, I don't find this particular instance to be problematic since the modal title clearly indicates which block you're locking. |
|
Great, thanks for the explanation @jameskoster! Also, I suppose since modals are typically fullscreen on mobile, that's probably another reason to avoid a modal if it has to maintain a connection to information outside of the modal.
Okie-doke, I'm quite possibly over thinking this one because I've been looking at the List View closely, so noticed a difference once I rebased against trunk and saw that the modal looked different. I'm happy to leave this, then, given that the block title is in the modal title 👍 |
|
I have gone through the call for testing: https://make.wordpress.org/test/2022/07/11/fse-program-testing-call-15-category-customization/#comment-2608 I noticed the various modals that showed up and how blurred the background area around it is. In general for a user interface the key is having subtile changes that give hints along the way to user. The current blur is too much of a change and makes it harder on the eyes working in Gutenberg. In general one does not want to bring too much contrast when working in any software. Think if OSX or Windows where to blur the background every time a modal is seen that one has to relate to before moving on. If one tries to do something else one is not able to and will need to find the modal that one does need to relate to before moving on. I do look forward to hearing more view points from other folks about the blur effect seen in the screen of it seems any modal that is opened. |
Our modals look a little tired:
current.mp4
This PR tries a few adjustments:
new.mp4
Perhaps some of these changes are more successful than others. Or perhaps none of them are :) What do you think?