BorderControls: Alternate approach to handling popover positioning #40874
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: -180 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
aaronrobertshaw
commented
May 6, 2022
Contributor
Author
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
While this draft PR still requires some polishing, it's functional and should serve to aid discussion on how we move forward in terms of positioning border control popovers.
Comment on lines
+33
to
+35
| const rightAlignedClassName = useMemo( () => { | ||
| return cx( styles.RightBorderControl, className ); | ||
| }, [] ); |
Contributor
Author
There was a problem hiding this comment.
Found while testing this the alignment of the right border control was out of whack when on tablet or mobile viewports.
Contributor
There was a problem hiding this comment.
Let's make a note to apply this fix even in case this PR doesn't get merged.
Comment on lines
+69
to
+74
| // We only want to apply adjusted popover content styles when on desktop. | ||
| const responsiveSide = useResponsiveValue( [ | ||
| undefined, | ||
| undefined, | ||
| __experimentalSide, | ||
| ] ); |
Contributor
Author
There was a problem hiding this comment.
I couldn't find the @include break-medium() equivalent using Emotion within the components package. This was my best guess but very happy to switch this out for any better ideas.
ciampo
reviewed
May 6, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you @aaronrobertshaw for coming up with an alternative approach to #40836 so quickly!
I personally prefer this approach, but I'll share a couple of thoughts/doubts:
- the concept of a sidebar feels more part of the “Gutenberg app”, and less of the
@wordpress/componentspackage (there isn’t no sidebar component, for example) — although these components already feel very specific and situational to the block editor (and already expose the__experimentalIsRenderedInSidebarprop). - having both
__experimentalSideand__experimentalIsRenderedInSidebaronBorderControldoesn't feel like the cleanest API, especially given that__experimentalSideonly has effect when__experimentalIsRenderedInSidebaristrue. @mirka and I recently discussed about how we're not massive fans of such "conditional" props.
A way to partially mitigate this last point could be to use the __experimentalSide also to tweak the popover's position when the component is not in the sidebar. I'm proposing this because currently the popover renders on top of other border controls (which is one of the main UX reasons for which we wanted to move the popover in the first place).
| style?: CSSProperties[ 'borderStyle' ]; | ||
| width?: CSSProperties[ 'borderWidth' ]; | ||
| }; | ||
| export type BorderSides = 'all' | 'left' | 'top' | 'right' | 'bottom'; |
Contributor
There was a problem hiding this comment.
A quick though, not necessarily related to the changes in this PR.
I wonder if we should consider switching to logical properties) — which would eventually require this type to become something like
Suggested change
| export type BorderSides = 'all' | 'left' | 'top' | 'right' | 'bottom'; | |
| export type BorderSides = 'all' | 'inline-start' | 'block-start' | 'inline-end' | 'block-end'; |
|
|
||
| export const borderControlPopover = css` | ||
| const sidebarBorderPopoverContentStyles = css` | ||
| width: 282px; |
Contributor
There was a problem hiding this comment.
Probably OK for a first iteration, but I wonder if there any way we can read this from a variable (or ever somehow measure it in the client at runtime?)
It would also be interesting if we could save this value as a local variable, and derive all right/left margins with formulas? (not sure it makes sense though)
| export const borderControlPopover = css` | ||
| const sidebarBorderPopoverContentStyles = css` | ||
| width: 282px; | ||
| margin-bottom: calc( ${ space( 13 ) } * -1 ); |
Contributor
There was a problem hiding this comment.
This style also feels a bit arbitrary — maybe we could add a comment explaining the reasoning behind it?
| margin-bottom: calc( ${ space( 13 ) } * -1 ); | ||
| `; | ||
|
|
||
| const borderControlPopoverContent = ( |
Contributor
There was a problem hiding this comment.
We could also add a comment here (or in this function's body) explaining that these margin rules are in place to align the popover to the sidebar
Comment on lines
+21
to
+25
| argTypes: { | ||
| __experimentalSide: { | ||
| options: [ undefined, 'all', 'left', 'top', 'right', 'bottom' ], | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Tweaking the __experimentalSide, even with the __experimentalIsRenderedInSidebar flag set to true, doesn't seem to change the appearance of the popover in the default story.
I wonder if we should disable the interactive controls for the story and/or create an ad-hoc story to showcase the "render in sidebar" scenario?
Comment on lines
+33
to
+35
| const rightAlignedClassName = useMemo( () => { | ||
| return cx( styles.RightBorderControl, className ); | ||
| }, [] ); |
Contributor
There was a problem hiding this comment.
Let's make a note to apply this fix even in case this PR doesn't get merged.
6 tasks
Contributor
Author
|
Closing this PR due to a new approach that will be adopted as part of #40740. |
Contributor
Author
|
Thank you for taking the time to review this approach and leave feedback @ciampo. Despite not proceeding with these, it's appreciated 🙇 |
Contributor
|
Don't mention it, Aaron! This was still a valuable approach that definitely deserved to be discussed / reviewed |
johnbillion
deleted the
update/border-controls-handle-sidebar-rendering
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
What?
Provides opt-in sidebar positioning for
BorderControlpopovers without exposing or passing through popoverProps via theBorderControl's public API.Why?
New work refactoring the
Popovercomponent requires the ability to position aPopovervia props rather than a class name.This is an alternate approach to #40836 after concerns about passing through
popoverPropswere raised.How?
__experimentalIsRenderedInSidebaristrueand__experimentalSideis set, theBorderControlwill generate new styles and apply them internally via the inner Dropdown'spopoverProps.BorderBoxControland updates it to leverageBorderControl's new approach (3aea82f)useResponsiveValueto adjust when the sidebar positioning should occur (i.e. only on desktop).Testing Instructions
npm run test-unit packages/components/src/border-control/test/index.jsnpm run test-unit packages/components/src/border-box-control/test/index.jsnpm run build:package-typesBorderControlpopovers, they should all be the narrower default width and their positions not modified.Screenshots or screencast
Screen.Recording.2022-05-06.at.4.03.25.pm.mp4