Try custom dialog for wide legacy widgets #33236
Closed
Conversation
This file contains 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
stokesman
added
the
[Feature] Widgets Customizer
|
Size Change: +1.3 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
stokesman
added
[Block] Legacy Widget
talldan
reviewed
Jul 7, 2021
| * | ||
| * @return {Object} A ref and resize observer element. | ||
| */ | ||
| export function useBudgeTopBy( objective, { isEnabled } ) { |
There was a problem hiding this comment.
It's a minor thing, but I feel like the naming could be iterated on. If this is just for use on the widget form it could take naming from that. useWideWidgetPopoverPositioning or if this could become something more generic useVerticalPopoverPositioning.
There was a problem hiding this comment.
I tried another name intended to describe the action useAlignTopWithinViewport but I'd be fine with changing to something like your first suggestion as this isn't intended to for other applications. I do like the idea that this hook could be removed at some point in favor of something more generic.
|
Popping this out of the Widgets project board since that board is primarily focused on 5.8. |
talldan
reviewed
Jul 7, 2021
Comment on lines
18
to
19
| const subjectRef = useRef(); | ||
| const objectiveRef = useRef(); |
There was a problem hiding this comment.
Another naming thing, I'm finding these aren't clear enough. I would regularly confuse the subject and the objective.
I think like other popover positioning utilities, it'd be good to stick with the term anchor for the element that the popover 'achors' to.
target is a term I find used more than objective, but could also call it popoverRef to use simpler language.
There was a problem hiding this comment.
I've renamed subjectRef to ref (it's the one returned by the hook for attachment on the element to be positioned) and objectiveRef is now anchorRef.
Comment on lines
32
to
49
| if ( ! boundsUpdaterRef.current ) { | ||
| boundsUpdaterRef.current = () => { | ||
| const { defaultView } = subjectRef.current.ownerDocument; | ||
| setBounds( { start: 0, end: defaultView.innerHeight } ); | ||
| }; | ||
| } | ||
|
|
||
| // Sets the bounds height and handles window resizes to update it | ||
| useLayoutEffect( () => { | ||
| if ( ! hasSubject || ! hasObjective || ! isEnabled ) { | ||
| return; | ||
| } | ||
| boundsUpdaterRef.current(); | ||
| const { defaultView } = objective.ownerDocument; | ||
| const updateBounds = boundsUpdaterRef.current; | ||
| defaultView.addEventListener( 'resize', updateBounds ); | ||
| return () => defaultView.removeEventListener( 'resize', updateBounds ); | ||
| }, [ hasObjective, hasSubject, isEnabled ] ); |
There was a problem hiding this comment.
There's potential to break this down into smaller hooks, e.g. const [ size, [ elementRef ] ] = useViewportSize().
Also noticed it subscribes to the objective viewport resize event, but returns the subject size. I think it should always use the objective as that's the element that needs to remain in bounds.
There was a problem hiding this comment.
I had a look a breaking into smaller hooks but wasn't convinced it was helpful for now (adds more lines). Though, there are around, I think, nine instance elsewhere in the repo a useViewportSize hook could be utilized so maybe it's a hook we should have.
I did as suggested for the listener.
7 tasks
|
The review is much appreciated @talldan. I'd considered it ready enough, it is drafted because it has an alternative and I thought it might be better broken into smaller PRs. The feedback makes sense and I'll get around to addressing it soon. Thanks! |
stokesman
force-pushed
the
try/legacy-widget-wide-no-popover
branch
from
c3e54b2 to
8236655
Compare
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.
Exploratory alternative to #33230, following up on #31736 and would close #32210 by obviating the need for it.
This is here for demonstration and if some of the changes here are desired would likely be better to break them off as their own PRs. The high-level changes with some reasoning:
Popoverto display wide widgets with a dialog (withuseDialogand a custom hook for positioning). Nice in that it requires no modifications to thePopovercomponent.How has this been tested?
Manually
Screenshots
customizer-widgets-wide-dialog.mp4
Types of changes
Enhancements
Checklist:
*.native.jsfiles for terms that need renaming or removal).