transitioning title attribute to render as tooltip, and adding readon…#4261
Conversation
…ly state for command bar items
|
This is basically adding a "readOnly" mode to contextual menu items (and should therefore also be added to buttons). It addresses the idea that a button can be focusable, but not actionable (as opposed to disabled which is treated as not really even there). It would probably have the same styles as disabled (though some design review would probably be useful). @joschect, @betrue-final-final : Has this approach been talk about/mocked up recently? |
| </TooltipHost> | ||
| ); | ||
| } | ||
| else if (tooltipContent) { |
There was a problem hiding this comment.
Nit: this should be on the same line as the closing {
|
|
||
| private _onItemClick(item: IContextualMenuItem): (ev: React.MouseEvent<HTMLButtonElement>) => void { | ||
| return (ev: React.MouseEvent<HTMLButtonElement>): void => { | ||
| if (item.readOnly) { |
There was a problem hiding this comment.
If this is added to command bar, it should also be added to the contextualmenu click handler.
There was a problem hiding this comment.
@joschect I added a comment on this prop that it's implementation is not supported by all components, like for contextualmenu and for button as micah as mentioned, then other teams can come in and add the functionality on these components as they need them. is that ok?
|
@micahgodbolt I'm a little hesitant about readonly being applied to non-input fields. Read only already has some connotation that the content could be editable. While in the case of buttons/clickable things you can't edit those. |
|
@joschect good point, readonly does already have a meaning in HTML. How about we just call it smoosh? j/k Regardless of the name, I think the state makes sense. |
|
@micahgodbolt so you want me to add this behavior to the button components also? @joschect how about, 'actionable'? because disabled is already taken unfortunately |
|
we usually want a Boolean prop to be false by default, so that changing it to true is the exception. I asked around on term ideas and one of my favorites was 'inactive'. Another was 'suppressed', or 'locked' (though locked seems like a specific reason for being inactive or suppressed). |
|
@micahgodbolt ok I will change this to inactive. But do I need to implement this in button or can it happen in a subsequent pr? |
|
If this is blocking you, then doing button later is okay. But that is technical debt that has to be cleaned up before you're able to move to the 6.0 CommandBar. |
|
@joschect I have made the edits you suggested |
micahgodbolt
left a comment
micahgodbolt
left a comment
There was a problem hiding this comment.
review comment, otherwise its fine. I like inactive
|
|
||
| let tooltipContent = ''; | ||
|
|
||
| if (item.title) { |
There was a problem hiding this comment.
Doesn't this add a tooltip to every command bar item? Is that intended?
There was a problem hiding this comment.
@micahgodbolt only if they provide a title attribute
There was a problem hiding this comment.
ah title attribute, yup. gotcha. was thinking title was the text in the button.
3 participants
Pull request checklist
Description of changes
Title attribute will now be rendered as tooltip, readonly state provided for command bar items
Focus areas to test
(optional)