Chrome: Adding the visibility selector #832
Conversation
youknowriad
added
the
General Interface
| const updatePassword = ( event ) => onUpdateVisibility( status, event.target.value ); | ||
|
|
||
| // Disable Reason: The input is inside the label, we shouldn't need the htmlFor | ||
| /* eslint-disable jsx-a11y/label-has-for */ |
There was a problem hiding this comment.
you mean completely disable it from the .eslintrc?
|
I love it. Ship it. I remember there being some concerns about the design of this, but can't recall the details. @folletto might remember? But either way that should not stop us from shipping this and iterating. |
|
I've added the information toggle, I'd appreciate a second look @jasmussen. Also, I wonder if we should use this "info" icon or design a new one (see calypso) |
|
Per slack chat, I think this is good to go. Let's keep the labels, no need to make them toggled. |
| /* eslint-disable jsx-a11y/label-has-for */ | ||
| return ( | ||
| <div className="editor-post-visibility"> | ||
| <span>{ __( 'Visibility' ) }</span> |
There was a problem hiding this comment.
Is the <span> necessary? Both here and in the <label> below.
There was a problem hiding this comment.
It's is not required here, but since the parent is "flex", I like the explicitness of having flex children. It's useless though for the label.
There was a problem hiding this comment.
It's is not required here, but since the parent is "flex"
Hmm, now I'm curious what is the default flex behavior for handling text nodes. 😄
There was a problem hiding this comment.
It seems to work properly though
There was a problem hiding this comment.
Seems to handle them independently only when they're broken up by other elements (notably not by react-text comments):
Explicitness seems good in these cases 👍
| return ( | ||
| <div className="editor-post-visibility"> | ||
| <span>{ __( 'Visibility' ) }</span> | ||
| <a className="editor-post-visibility__toggle" href="" onClick={ this.toggleDialog }> |
There was a problem hiding this comment.
If it's not navigational, it shouldn't be an anchor. Core has a .button-link class which gives an appearance close to but not exactly the same as a link which we could use as a base.
|
Is this ready to go? |
|
👍 👍 from me. If you've addressed all the code comments, visually and interaction wise this is good. |
It was mostly in terms of the clarity of the interactor: in the current solution the color, and even more the underline, should be a strong enough signifier. 👍 |
|
Thanks Davide, appreciate it. |
This is largely inspired from Calypso.
Open Questions
FormFieldset,FormLegend,FormRadio,FormInput?