-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Image block: fix replace link control styling #33326
Conversation
Size Change: +27 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
This comment has been minimized.
This comment has been minimized.
@javierarce a quick 👀 from a design perspective would be good here if you have any time. |
This looks much better, @getdave. I just saw a couple of things:
I also discovered that if you enter a Base64 encoded image, the URL that appears in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works as described. Code looks good. 👍🏻
Thanks for the review @javierarce!
I've added a consistent 8px margin in both edit and preview mode.
Removed that.
I set a hardcoded 200px width which avoids this.
I don't think we should attempt to "fix" this here but in a follow up. Essentially we do have much more data available but it's not being passed to the component so it isn't being displayed.
I think that should be resolved by the fixed width above. |
…renders by default This preserves the existing functionality of the control.
As requested by designer in #33326 (comment)
f8dfeda
to
691f276
Compare
Rebasing FTW |
The changes look good to me! 🚀
Ok! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for working on this, @getdave.
Tests failing probably needs a rebase. Please could someone merge this so it makes 11.1 RC? |
@getdave this works as expected. Fine for me :). Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well! 🚢
Tests finally passed. I didn't do anything so I guess these were upstream issues on |
@youknowriad This might be a good one for cherry picking in cc @gwwar who I think is shadowing the release. |
Changed the milestone to consider cherry-picking this. |
Thanks @youknowriad. Wasn't sure whether to go ahead and do that without confirming with the release lead first. |
* Conditionalise display of wrapping “tools” div * Align items visually to top of control below “Current media URL:” text * Extract default settings to root component scope to ensure tools div renders by default This preserves the existing functionality of the control. * Add space between label and input As requested by designer in #33326 (comment) * Fix popover resizing when switching between edit and preview mode
Description
The DOM and styling updates to
<LinkControl>
necessitated by the introduction of the the rich link previews feature caused the Image blockReplace
flow dropdown to suffer a visual layout regression.This PR resolves this by aligning everything nicely again. See screenshots below.
Fixes #33096
How has this been tested?
Replace
on the Image block.Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).