Media Replace Flow: Vertically align the URL #58621
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
scruffian
added
[Type] Bug
scruffian
requested review from
ellatrix,
artemiomorales,
michalczaplinski and
ajitbohra
as code owners
|
Size Change: -6 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Feb 3, 2024
There was a problem hiding this comment.
Thanks for the PR!
Personally, I'm hesitant to introduce a new title prop. This is because I think that to solve this layout issue, we would force consumers using this component to add their own data to pass to the title prop.
What do you think about simply aligning them vertically and in the center?
.block-editor-media-flow__url-input .block-editor-link-control__search-item-header {
align-items: center;
}
|
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 If 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. |
|
I agree with @t-hamano in the sense that adding a prop to a component to fix an UI problem does not seem right 🤷🏻 The align solution looks like an UI solution to me too. |
|
I think it'd be nice to have the title of the attachment, where we have the title of the link, but if folks don't think it's worth the extra lift, I'm ok with vertically centering. |
|
If the UI demands extra data then shouldn't we add additional props to support it? I tried to fetch the data in the replace media flow component, but that component doesn't like importing from the core data store. |
|
You're corect @scruffian i just don't know why the UI demands a title - the title does not look great when it's a long weird file name. In the referenced issue the bugs are layout related and there is no title before the offending changes were made. With a title the label of the component "curent media URL" becomes a bit disconnected too. |
|
When we link to pages and posts we show a title, so using the title is consistent with that. I agree that it doesn't look great when the title is autogenerated from a file name. |
|
The For example, in addition to this PR change, how about making the following changes to diff --git a/packages/block-editor/src/components/link-control/style.scss b/packages/block-editor/src/components/link-control/style.scss
index 7b6470df43..557145aeac 100644
--- a/packages/block-editor/src/components/link-control/style.scss
+++ b/packages/block-editor/src/components/link-control/style.scss
@@ -177,7 +177,7 @@ $block-editor-link-control-number-of-actions: 1;
.block-editor-link-control__search-item-header {
display: block;
flex-direction: row;
- align-items: flex-start;
+ align-items: center;
margin-right: $grid-unit-10;
gap: $grid-unit-10;I also think that if we add the title property, we also need to update the reference. |
Same here. Let's fix the alignment and circle back on attachment titles. |
scruffian
force-pushed
the
add/media-replace-title
branch
from
d64d872
to
73dd843
Compare
scruffian
changed the title
|
I have updated the PR with the vertical alignment, and the screenshots. |
|
Flaky tests detected in 73dd843. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7837343720
|
t-hamano
approved these changes
Feb 9, 2024
There was a problem hiding this comment.
LGTM!
I believe that simply aligning them vertically is a simple and logical approach in the sense that it preserves the existing MediaReplaceFlow layout. If we need a title in the Image, Audio, or Video block in the future, we can consider adding the title prop to the MediaReplaceFlow component at that time.
|
Sorry I forgot to copy the props into the merge commit again :( |
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?
Vertically aligns the URL inside LinkControl, when it's used in the MediaReplaceFlow component.
Why?
This stops the UI from looking broken when there is a missing title. Fixes #58593.
How?
CSS
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast