Adds edit in place for menu name #25343
Conversation
|
Size Change: +3.08 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
draganescu
requested review from
ajitbohra,
chrisvanpatten,
Soean,
talldan and
youknowriad
as code owners
draganescu
changed the base branch from
master
to
try/navigation-use-filter-add-controls
draganescu
force-pushed
the
add/edit-in-place-menu-name
branch
from
f39ff57 to
da11745
Compare
draganescu
force-pushed
the
add/edit-in-place-menu-name
branch
from
da11745 to
377e0cf
Compare
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
|
In my testing:
|
noisysocks
added
[Feature] Navigation Screen
[Type] Enhancement
I also think this to be an unexpected behavior (edit in place) and I would rather a rename action with a save action appearing on edit. Or, maybe just a regular textfield, which gets saved along with all the menu. Edit: @noisysocks the textfield appears only when the navigation block is selected. At some point we removed the auto select behavior. I tried to have the edit in place component now show up, or the edit not working but it works at every refresh or click to edit. Tested both in Chrome and FF. What am I missing? |
draganescu
force-pushed
the
add/edit-in-place-menu-name
branch
from
9b86d58 to
262b10e
Compare
draganescu
requested review from
adamziel and
tellthemachines
as code owners
draganescu
force-pushed
the
add/edit-in-place-menu-name
branch
from
262b10e to
62547ab
Compare
packages/edit-navigation/src/components/layout/use-navigation-block-with-name.js
Outdated
Show resolved
Hide resolved
|
I tested this and it seems that the state of the edit in place control is problematic as seen below: edit-in-place-state.mp4This is the same problem I've encountered when wrapping the state management in the Also @grzim when using the arrow keys to move between letters the focus should not move to another button. |
draganescu
requested review from
guarani,
gziolo,
mkaz,
mmtr,
nerrad,
noahtallen,
oandregal,
ntwb,
spacedmonkey,
swissspidy and
TimothyBJacobs
as code owners
grzim
force-pushed
the
add/edit-in-place-menu-name
branch
from
8ba2479 to
faa079a
Compare
|
This seems to work well. I think we could add a transition between the resting state and the focus state to help avoid the jarring movement of the toolbar.
In the context of #28675, I was thinking we could have the menu name input in the sidebar. If this lands though, I think having it in the sidebar might not be necessary. |
There was a problem hiding this comment.
Sorry, lots of comments! Most were around the EditInPlaceControl, and I'm wondering if we should just go for @shaunandrews' suggestion of using a normal input in the sidebar. I think this EditInPlace input is going to be really hard to get right.
Going for the sidebar would mean using the InspectorControls component instead of BlockControls.
| return ( | ||
| <> | ||
| { ! edit && ( | ||
| <Button |
There was a problem hiding this comment.
I don't think it'll be clear what this button does for screen reader users given its only label is the value of the input, which can be removed entirely.
Adding an aria-label would probably remediate this. That's something that should be customisable using a prop. The input field is also completely unlabelled, so again, it should use this value from a prop.
The way this component has a label prop that's really the value is probably wrong. I think it'd be best to repurpose label for the aria label.
There was a problem hiding this comment.
Tested this and it works. I love that the name resetting / not saving in the UI problem is solved by adding the filters in the right place and the rearrangements of the code help too. Left some comments to make the PR a bit smaller (move the suggestions function rewrite to a separate PR) and a couple style related.
| setEdit( false ); | ||
| onUpdate( value ); | ||
| } } | ||
| onKeyDown={ ( event ) => { |
| width: 50%; | ||
| transition: width 0.3s; |
There was a problem hiding this comment.
I'm not sure I understand. If the concern is about longer menu names, then we can truncate at a reasonable length:
Otherwise, I'd expect the transition to feel something like this:
There was a problem hiding this comment.
I think github had a glitch as my original review comment disappeared. My comment mentioned that the animation doesn't work as expected when the menu name is long, it becomes smaller and then expands:
I'd expect it to go smoothly from one size to the other.
There was a problem hiding this comment.
The problem is that the component changes from being a button, that automatically takes the width of its contents, to being an input, that by default takes a width of 20 characters (that can be changed by using the size attribute). So what's happening is we're transitioning to an input with a smaller width than the button, and then because the input's initial size is even smaller than its final size, we see that jump.
There are two ways to approach this: either we set the input size to equal value and it'll grow or shrink when edited, and always be exactly the same size as the button, or we set a fixed width for both button and input, but then I'm not sure there's any point in making it bigger when it's editable 🤔
Also: we don't need the &.small rule (or the small classname at all); transition and the initial width value can be set directly on .components-edit-in-place-control__input, and then we won't need to repeat transition in &.large either.
There was a problem hiding this comment.
I have moved this component to a separate PR #28924.
In my opinion fixed width of a button and input could be problematic for users when they have longer menu names.
@tellthemachines in order to make an editable area that stores users input and keep the size of text we could either add some js logic to enlarge input while typing or to use span with contenteditable property set to true:
<span
role="textbox"
aria-label={ editNameInputLabel }
ref={ inputRef }
className={ inputCssClasses }
contentEditable={ true }
onInput={ ( event ) => {
const _value = inputRef.current.innerText;
setValue( _value );
onChange( _value, event );
} }
dangerouslySetInnerHTML={ { __html: value } }
onFocus={ () =>
setInputCssClasses( withTransitionCssClass )
}
tabIndex="0"
onBlur={ () => {
const _value = inputRef.current.innerText;
setValue( _value );
setInputCssClasses( withoutTransitionCssClass );
setIsEdit( false );
} }
onKeyDown={ ( event ) => {
if ( 'Enter' === event.key ) {
cancelEvent( event );
if ( inputValidator( value ) ) {
setIsEdit( false );
setValue( value );
onUpdate( value );
}
}
if ( 'Escape' === event.key ) {
cancelEvent( event );
setValue( initialValue );
setIsEdit( false );
event.target.blur();
}
onKeyDown( event );
} }
/>
The input element is more stable, it is easier to add transitions and work with it in general. On the other hand, contenteditable makes the size adjusting trivial.
There was a problem hiding this comment.
We don't need JS logic to adjust the input; its size attribute does that automatically. Try setting size to value and you'll see what I mean 🙂
There was a problem hiding this comment.
Nice idea @tellthemachines :)
Here value represents the variable that is displayed inside an input, so the valid expression to pass to size attribute would be String(value).length. Nevertheless, somehow it is not adjusting properly, and not all chars are visible
😕
There was a problem hiding this comment.
Yeah, String(value).length., of course 😅
Any CSS attached to the element will override size, so you'll have to remove the widths and transitions for it to work.
There was a problem hiding this comment.
surely I did;) I will make this component available for testing and you can see what I am talking about.
| } } | ||
| /> | ||
| ) : ( | ||
| <Button |
There was a problem hiding this comment.
The use of Button causes a console warning:
Using custom components as toolbar controls is deprecated. Please use ToolbarItem or ToolbarButton components instead. See: https://developer.wordpress.org/block-editor/components/toolbar-button/#inside-blockcontrols
It should be implemented as a ToolbarButton component
There was a problem hiding this comment.
I have moved edit-in-place to the another PR. Here the simple input is being used
| return ( | ||
| <> | ||
| { isEdit ? ( | ||
| <input |
There was a problem hiding this comment.
Similar to the issue with the Button, this also causes a warning. It would have to be implemented as a ToolbarItem.
There was a problem hiding this comment.
altered to <ToolbarItem as="input"
| width: 50%; | ||
| transition: width 0.3s; |
|
There seem to be a lot of unrelated commits on the branch, not sure if something went wrong with a rebase. |
grzim
force-pushed
the
add/edit-in-place-menu-name
branch
from
d3260f6 to
198fa44
Compare
Advances #Advances #25233
Based on #25329
Fixes: 24581
Description
Adds an edit in place control. Adds said control to the Navigation block's toolbar used in the Navigation editor via a filter.
How has this been tested?
Tested locally.
Screenshots
Types of changes
Non breaking changes. An alternative implementation is how @noisysocks conditionally includes the inspector controls in here.
Checklist: