-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Adds always on display of media URL #19504
Conversation
packages/block-editor/src/components/media-replace-flow/index.js
Outdated
Show resolved
Hide resolved
I like it. Two suggestions:
|
b20a223
to
6d5209b
Compare
@draganescu Hai there! Thank you for working on this fix 🙌 I'm seeing some wonky UI from this PR. Just wanted to follow up to see if we were able to apply the changes suggested by @shaunandrews I also noticed that the Thanks again! |
hi @ItsJonQ and thanks for testing! Indeed, the |
2cdbc0c
to
d41f1e3
Compare
This now works as expected but I am not happy with the implementation, as it still uses the old
|
Hi @draganescu , I'm not sure I follow what you mean by the specific requirements you list. What is the second "Apply" state, and how would a LinkControl be closed? Is this referring to the ability to have some option to "Cancel" out of the editing input and return to the preview state? |
Yes @aduth , I should have explained better. But briefly, if you look at the GIF up in the description of this PR, the interaction is in two steps:
And this is very similar to how the Second, if I want to quit editig and get back to viewing the link, |
I think we should add an explicit "Submit" (aka "Apply") UI. We could make this optional. It would sit next to the "Reset" button.
In this scenario you have (for example) clicked "Edit" but then decided you want to go back and not edit? But you can't do that. Would the Apply button solve this issue? I think both the above are solvable but should be in a separate PR. |
@draganescu Thanks for clarifying. These are things I'm very interested in as well.
If I understand this correctly: The applied editing does exist as handled as a form submission, but there's nothing in the UI to indicate this (or to click on for submitting). It relies on the Enter key press to submit/apply. If my interpretation is correct, I think this can serve as a testament that #19971 would be a good revision to pursue to improve this clarity.
There was some similar challenges for implementing this in the paragraph (#19462 (comment)). You might be interested in the discussion in #20007. It might be tricky for your specific use-case. My first thought was that "Escape" could toggle the internal state of Using focus as a basis for the editing state might work, but it needs to account for the additional action buttons shown alongside the input (currently "Reset", maybe in the future "Submit" and/or "Advanced Settings"). There could also be an explicit Cancel button in here as well. The problems here are that: (a) the space is limited (related) and (b) the goals of #20007 might be such that we don't want the preview state to be reachable at least in the context of paragraph link editing workflows. |
Thinking more about Cancel:
|
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.
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
Hi @shaunandrews I've added all your suggestions and it looks pretty neat. Let me know of any other tweaks :) |
Per previous comments about LinkControl applicability here, not that as of #20052, at least one of the issues you mentioned should now be resolved (a "Submit" button is always shown in the editing state). |
37681e7
to
f160f8d
Compare
This is looking good now, but it does not use |
f160f8d
to
cd5f1c6
Compare
@getdave @aduth I have implemented this UI using the
Do you think these are good modifications? Is the |
packages/block-editor/src/components/link-control/search-input.js
Outdated
Show resolved
Hide resolved
@Soean could you re-review and see if everything works as expected? :D Thank you 💯 |
@draganescu |
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.
Nice work on updating this to use LinkControl
. It's great that we're stress testing this component. I hope it wasn't too painful!?
I noticed a few things that caught my eye. Let me know if anything doesn't make sense.
onKeyPress={ ( event ) => { | ||
const { keycode } = event; | ||
if ( keycode === ENTER ) { | ||
selectSuggestionOrCurrentInputValue(); |
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 caught my eye. I'm sure this code got removed in earlier PRs on LinkControl
. Does the action not get handled by the onSubmit
handler?
If not then we could consider adding a comment here to indicate why we're having to double up here as it wasn't immediately apparent to me.
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.
There is this comment in the URLInput component which made this neccessary:
// If the suggestions are not shown or loading, we shouldn't handle the arrow keys
// We shouldn't preventDefault to allow block arrow keys navigation
As in this context we explicitly disable suggestions then we need to also handle stopping key propagation to observe typing. Otherwise The pop up closes.
Would you think there is a better way around this?
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.
I would have to also take care of the Escape button as now tests are failing b/c the link editor is not being dismissed on ESC anymore :( ugh.
packages/block-editor/src/components/link-control/search-input.js
Outdated
Show resolved
Hide resolved
@Soean good catch. After rebasing the new G2 UI was added and now it is a bit worse :) The button is not cut, the whole link editor is having an invisible scrollbar. I wonder what is the best solution here. I would simply hide the link on mobile, to make sure edit in all languages definitely fits. Another option would be to wrap the button ... but that raises other issues. |
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.
Looks great now!
We need to change $grid-size-large
to $grid-unit-20
, see #20529.
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-replace-flow/style.scss
Outdated
Show resolved
Hide resolved
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
…scss Co-Authored-By: Sören Wrede <soerenwrede@gmail.com>
…scss Co-Authored-By: Sören Wrede <soerenwrede@gmail.com>
…scss Co-Authored-By: Sören Wrede <soerenwrede@gmail.com>
…scss Co-Authored-By: Sören Wrede <soerenwrede@gmail.com>
18497c4
to
f24307d
Compare
… usage in a dropwdown for MediaFlow
…he new grid variables.
Thank you @shaunandrews for the tidy up tweaks. I am so glad this is finally merged :) |
Are we able to truncate the URL in the middle so that it appears as |
Good suggestion @noisysocks ! I'll open an issue with that in it. |
Done here it is: #21100 |
Description
Implementation of one of the variants in #18992 about always showing the URL of the current media in the media replace control.
How has this been tested?
Tested locally.
Screenshots
Types of changes
Updated
MediaReplaceFlow
andLinkViewer
.This should eventually use the new
LinkControl
instead.cc: @shaunandrews