G2 visual followup #21476
G2 visual followup #21476
Conversation
jasmussen
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
A few screenshots of work in progress. I tried removing the triangle, because we might not be able do the negative leftwards shift of the toolbar. Before:
After:
I fixed the saving gradient. Before:
After:
|
|
Size Change: -161 B (0%) Total Size: 903 kB
ℹ️ View Unchanged
|
|
Tweaked alignments. Before:
After:
Also did this for block navigation:
|
|
Added animation to toggle buttons:
Unfloated the notice button to fix an issue it regressed. These notices should be redesigned separately:
|
|
Pushed a fix to set a default popover direction. This adds some much needed consistency:
|
|
I added spacing to popovers, and a new "isPrimary" prop to the popover which will paint it dark and at elevation zero:
This gets us very close to the mockups. By default, the popover still uses the light gray shadowed treatment:
The primary prop should only be used when the popover is in the context of block toolbar UI. |
jasmussen
added
[Type] Code Quality
|
I like the new popover styles.
Why? |
|
I suppose I should've finished that sentence with "... or if you know what you're doing". I think of it as an extension of the block toolbar, being made of the same material and being coplanar with it: extending at the same z depth layer and therefore subject to the same "physics". When a popover is opened from the top toolbar, on the other hand, it's not the same material and it will actually overlay the toolbar itself and often, the sidebar. That beckons a higher z depth, which suggests the shadow. Extending from secondary (to the block) UI also means the dark border wouldn't connect in any meaningful way. That's how I think about it, in any case! |
|
Love the improvements! What if the popover outlines were the same, and the popover was positioned slightly lower and separate from the toolbar? Probably discussed before, so sorry if I missed that. :)
|
|
@ellatrix Thanks!
But that is exactly what this PR does, or am I misunderstanding?
|
|
Ooooh, perfect then! |
Ah, I missed this bit. Is there a need to differentiate? Why not make all popovers like this? |
I think so, yes. I went into a little more depth in my comment to Zeb (#21476 (comment)) — but it boils down to this: by default the popover component should behave as a popover: it overlays content. But in the specific case of popovers that extend from the block toolbar, these are so literally close to the block itself, that they are at what I would call "elevation zero". Not talking CSS z-index here, but rather something similar to Material's "elevation" principle: https://material.io/design/environment/elevation.html Another way to think of it is just that those popovers just receive more emphasis because they are primary UI. Same principle as when you have a single Button component with an isPrimary attribute. |
| @@ -30,6 +30,7 @@ const DEFAULT_ALIGNMENT_CONTROLS = [ | |||
|
|
|||
| const POPOVER_PROPS = { | |||
| position: 'bottom right', | |||
| isPrimary: true, | |||
There was a problem hiding this comment.
Wonder if we should use a more descriptive name here because I don't feel primary / secondary is what should determine the weight. Maybe something like "isFloating"? Or "isBlockUi" since these are used for blocks only? Just a thought.
There was a problem hiding this comment.
I'm super duper open to changing this, and now is the best time to do so. Which is your preference? We could also go in a different direction, like isEmphasized?
|
It seems weird to me to use shadows in some popovers but not others. When I first saw the G2 mockups, I thought we were moving away from shadows entirely and that everything would use the high contrast borders that the block toolbar uses. It seems weird to me to use shadows in some places but not others. If some popovers are going to have shadows, why not also the toolbar? I would think of it as being at a higher elevation than the content being edited, yet it has no shadows. It seems like an inconsistency to me to use a mix of some floating elements having shadows and others not. Also, I don't really get the idea of emphasizing certain popovers over others. All the popovers are closed by default, so different styling on one versus the other isn't really going to make a difference on where your eyes are drawn to. When you open the more menu in the top toolbar, your attention is focused on the more menu popover, so shouldn't it appear just as emphasized as any other popover? I'm not convinced other users are going to understand why some popovers look different than others, either. Without having seen your comments, I would have assumed it was just a visual consistency bug. That said, if others disagree with me, I'm okay with this PR merging as-is, design-wise. I'm happy to have the popovers looking more consistent with the block toolbar UI, even if it's only some of the popovers. |
To quote a friend of mine: consistency is nuanced. Honestly, the term "popover" is not one a user is likely ever going to see or even think about, it's just what the underlying component is called. The top toolbar and UI outside the canvas, the borders and separators are intentionally lighter because it is secondary UI and because it does not have to visually distinguish itself from the theme. Menus that extend from this UI are consistent with this UI. And inside the canvas, there's the block toolbar. This UI has been designed not only to be primary, but to work in any context background:
Yes, technically alignments appear in a popover — but just like the "Parent Button" is made from toolbar material, so are the menus that extend from the toolbar. |
|
Renamed isPrimary prop to isAlternate. Also used the smaller plus for the big appender button. Looks good, props Pablo for tip:
|
Fixes #21316.
jasmussen
force-pushed
the
try/g2-more-followup
branch
from
7fed8fb to
53ce532
Compare
|
Addressed feedback, rebased, updated snapshots. I feel like this branch is in a solid solid place, and none of the disagreements feel like ones we can't revisit later on, if they turn out to be problematic. I would definitely still welcome thoughts and such, and most definitely testing. But otherwise, this is ready for final review! |
|
Thank you Zeb. I forgot to test again RTL and the new default popover direction, so I'll test and verify that before merging. |
|
I wanted to make sure that the new default behavior for popovers to open down and rightwards was properly handled in RTL language directions. It is, it defaults to down and leftwards:
In that vein, I'm going to merge this one. I wanted to make a note that the change in #21476 (comment) to remove the triangle indicator is one that is worth revisiting if/when we look at the mover control behavior again. I consider it a delightful aspect of the G2 visuals, and one that can provide great character when it does indeed point to the left margin of the block. So I would love to restore it when we can. |
Followup to #21476, which defaulted popovers to open down and right. This restores the "down and center" behavior for the inserter as used in-canvas.
Followup to #21476, which defaulted popovers to open down and right. This restores the "down and center" behavior for the inserter as used in-canvas.
|
Nice work, Joen! |
This PR fixes #20575 and addresses feedback, plus some more.
Aside from the visual considerations, I'd like feedback on the popover prop code, I'm sure I could've done that in a nicer way.
I would also love some RTL feedback on the default popover sirection, to see if we can maybe change the default per locale.