Try: Fix lightbox URL popover position. #58600
Merged
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -454 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Feb 2, 2024
There was a problem hiding this comment.
I think there are three solutions.
In the short term:
Opt in offset prop to URLPopover component.
diff --git a/packages/block-editor/src/components/url-popover/image-url-input-ui.js b/packages/block-editor/src/components/url-popover/image-url-input-ui.js
index 0eaea31491..ed958e45b1 100644
--- a/packages/block-editor/src/components/url-popover/image-url-input-ui.js
+++ b/packages/block-editor/src/components/url-popover/image-url-input-ui.js
@@ -298,6 +298,7 @@ const ImageURLInputUI = ( {
</MenuGroup>
)
}
+ offset={ 13 }
>
{ ( ! url || isEditingLink ) && ! lightboxEnabled && (
<>URLPopover components can pass props to internal Popover components. Popover component supports offsets. This offset is probably why the Dropdown component has a reasonable spacing.
In the long term:
Refactor with Dropdown component.
Refactor the components related to the image link UI, namely the ImageURLInputUI and URLPopover components, using a Dropdown component.
This may require major refactoring internally and will also impact backwards compatibility.
In the longer term
It may be replaced by a LinkControl component.
|
Nice, I used Offset, that works well:
Thank you. What do you think, can it land? |
t-hamano
approved these changes
Feb 2, 2024
There was a problem hiding this comment.
LGTM!
This time, the change was made to the ImageURLInputUI component, but this component is only used in two places: the Image block and the Media & Text block. Therefore, there should be no backwards compatibility issues or unexpected issues.
It's working properly in the mobile viewport as well.
I think this is a clear first step to improvement, so I think it's okay to ship it.
MaggieCabrera
added
General Interface
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.
What?
The UI to untoggle the lightbox was recently vastly improved, moving to the toolbar. But the position is incorrect:
Like other toolbar dropdowns, the popover itself should be moved down 12px:
This PR implements a hack to make it appear the same:
It's not clear to me if we want to land this hack as is, so marking as draft. From a glance it seems the
Dropdowncomponent automatically adds these 12px, rather than relying on margin, whereas since this URL dialog is not using that component, it positions itself at the bottom of the URL button itself.What's the best fix here?