Link UI: polish lightbox pieces. #58666
Conversation
jasmussen
added
[Type] Bug
|
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 SVNCore Committers: Use this line as a base for the props when committing in SVN: 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: -397 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
I'd be happy to pull out any conflicting changes and make this only about the URL pieces. |
I like that this effort is simpler and actionable; a quick fix, not a refactor. As I detailed #58505 (comment), we should follow-up with using LinkControl everywhere, as detailed in #50893. |
|
I see, so let's merge this PR first and follow up with #58505. I made a slight adjustment with cbbc4f4. I removed the icon wrapper div and removed unnecessary class names and styles. For the description, I followed the style of the menu item, added a 5px margin-top and changed the text color from Before:
After:
Let me know what you think of this. |
|
Thanks for all the reviews, and thanks for pushing. Please feel free to both continue pushing, and to press the green button when you're ready. It isn't clear to me what is the best order to merge these changes, so I'm happy to defer to you.
While I do appreciate the code cleanup (feel free to merge, we can always polish later), the line-height was intentional. Specifically, we need the title (Expand on click) and the helper text (Scales the image...) to not be taller than 32px at most. When that's the case, the entire box is a total of 48px tall which matches exactly the height of the block toolbar. This gives some shared DNA in terms of the "materials" used. As noted, not a blocker, and it might be nice to look at title+helptext in a more holistic way. From what I can tell, even after these changes the metrics are not the same as they are in this dropdown:
|
|
Thank you for the detailed explanation. Perhaps the focus is on this margin-top? If you don't mind, is it possible to revert the unintended changes I made? Of course, you are welcome to revert my entire commit 👍 |
|
I have to step away for some dinner prep and such, if you are able to do the revert, feel free to, otherwise I'll look tomorrow. But also as noted, it seems fine to land as is, I'm happy to follow up on the height separately if need be. |
There was a problem hiding this comment.
While I do appreciate the code cleanup (feel free to merge, we can always polish later), the line-height was intentional. Specifically, we need the title (Expand on click) and the helper text (Scales the image...) to not be taller than 32px at most. When that's the case, the entire box is a total of 48px tall which matches exactly the height of the block toolbar. This gives some shared DNA in terms of the "materials" used.
Thanks for letting me know why! I removed the top margin of the helper text. I think it matches the original state of this PR, with the only exception being a change in the color of the helper text.
I think we can ship this PR, but if I've missed anything please comment.
|
Flaky tests detected in cbab38b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7809614060
|
|
I'll merge, and I'll follow up, just let me know. |
|
Noting that this PR doesn't have an associated issue. It's not the only case though. It appears that PRs with no accompanying issue are a recurring trend in this project. I would like to remind everyone that the guidelines of the Gutenberg project and the spirit that always inspired contribution to the WordPress.org project require any non-trivial pull requests to be preceded by a related issue. Allowing for broader discussion, in an issue or a Trac ticket, is fundamental in an open-source, collaborative, project. I'm not sure ignoring that guideline encourages contribution to this project in any way and I'm not sure it's in line with the principles of the WordPress.org project. It also triggers the perception that there is a 'double standard' in the Gutenberg project, where some contributors feel they can do whatever they want while other contributors are asked to follow a more strict process. Lastly, I'm not sure this is the most efficient way to contribute, as it leads to overlapping issues and PRs. That said, I'm not sure a closed PR is the best place for this kind of conversation. I'll consider to move these considerations to a more appropriate place. |
|
Oh you're right. I've updated the description to include the pertinent issue and conversation. |
If I'm not wrong, the issue you linked to: #54916 was closed as completed on Jan 31, 2024. This PR was created on Feb 5, 2024 so it's actually a PR associated to a closed issue. For the future, in such cases I'd kindly ask everyone to either reopen the supposedly associated issue or create a new one, please. Also, neither this PR nor #54916 nor the PR with the mentioned code review comment have any accessibility related GitHub label, while they do impact important accessibility features like the inputs border and focus style. |
| margin-left: 0; | ||
| margin-right: 0; | ||
|
|
||
| &:not(:focus) { | ||
| border-color: transparent; |
There was a problem hiding this comment.
This caused the text link editing popover to have a transparent border on the link combobox:
There was a problem hiding this comment.
Thank you for finding that. Perhaps the increase in CSS selector specificity for adding transparent borders caused this border to disappear unintentionally. In #58505, restoring the default border of the URL input field, i.e. removing this style itself, should resolve this issue.
What?
This addresses as yet unfinished design work that went into #54916. Specifically the followup items mentioned here: #57608 (review)
The metrics in the link UI for lightbox can be improved:
and the focus style is broken
This PR fixes both. First it addresses the metrics:
And the focus style is repaired too:
Testing Instructions
Insert an image, test the URL focus, test the lighbox option.