Skip to content
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

Render commandpalette checks #4510

Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2018

This adds a new renderer to the Command Palette to render the isToggled state for commands. One consequence of this is that we can remove several instances of having a different labels for the command palette vs the menus. Another consequence of this is that several places where keyboard shorctuts were incorrectly labeled (due to the isPalette workaround) are now fixed. Before:
image

After:
image

Addresses #3630.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 2, 2018

Do you think having icons in the command palette is generally a useful thing? Maybe we should add that to Phosphor core? I know @jasongrout brought this up on Gitter, but I don't think I was fully envisioning what he meant.

We could add the icon support to Phosphor is a backwards-compatible way.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 2, 2018

Yeah, this was prompted by the discussion on Gitter earlier today, but I have been kicking around this idea for a while.

I do think this would be a generally useful thing for the base command palette. Of the state attributes for commands, isEnabled, isVisible, and isToggled, only the latter is not reflected in the command palette rendering. If we rely on that information to display things for the user in other contexts (like menus), we have to do awkward things like the isPalette arg + conditional labels for the command palette to communicate that information in that context.

Furthermore, workarounds like having context-signifying args has consequences in that keyboard shortcuts are not always correctly rendered (I reference this problem in phosphorjs/phosphor#325).

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 2, 2018

@tgeorgeux You just adjusted the heading padding, do you want to do that for the items as well?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 2, 2018

Yes, I'm going to fork it and play around with the CSS to center the check a bit more.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 2, 2018

Do you think having icons in the command palette is generally a useful thing?

I think that having icons is not as crucial as exposing the toggled state to the user. I'll let the UX folks weigh in on that one.

Edit: actually, I think it's not as important...

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 2, 2018

Yeah, by icons, I mostly mean the toggled-state.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 2, 2018

Note that for menu items, the icon node serves double duty to show an icon or a toggle check. We can do the same with the command palette. Back in the day we decided against icons in the command palette for reasons I can't remember.

@ian-r-rose ian-r-rose force-pushed the render_commandpalette_checks branch from b3cea75 to e0c5793 May 2, 2018
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 3, 2018

@ian-r-rose
Here's the centering I was looking for:
screen shot 2018-05-02 at 3 13 52 pm

I found the relevant CSS sections:
2
1

Updated to center checkmark with proper 4px padding.
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 4, 2018

@ian-r-rose I updated the commandpalette.css I'm good to merge. Did we want to open an issue to continue the discussion related to the icons?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 4, 2018

I am happy with this as it is, but we should decide whether we want to put this into @phosphor/widgets as an alternative (@sccolbert?).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2018

I'm happy with this too. We can decide whether to move it down the stack to phosphor later, right?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 4, 2018

Yep, we can always do that later.

@jasongrout jasongrout merged commit 7bc2df2 into jupyterlab:master May 4, 2018
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2018

Thanks!

@jasongrout jasongrout added this to the Beta 3 milestone May 4, 2018
@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 4, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2018

In playing with this, and with the view menus, it may be good to have a signal to the user that the option is a toggle when it is untoggled. For example, I think some UIs have an empty box (like the empty box for a checkbox control) to indicate that it could be checked, but isn't. As it is now, the untoggled command looks just like an ordinary command until it is toggled.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 4, 2018

Checkable menus typically don't render anything when they are unchecked. This goes for boths Windows and OSX.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 4, 2018

I really think we should have a discussion about whether command palette icons should be a core feature before this gets lost in the dustbin of issue backlogs.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2018

And are you saying "palette icons" includes the toggled state, or are you distinguishing between icons and the toggled state?

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 4, 2018

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 4, 2018

I'm not distinguishing between icons and checks. See my comment above about how menus deal with icons and checks. We can do the same thing in the palette.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 4, 2018

I personally think that toggles show valuable state, but icons may clutter up the command palette. I wouldn't be opposed to trying it, but my guess is that I'd find arbitrary icons clutters things up.

@tgeorgeux
Copy link
Contributor

@tgeorgeux tgeorgeux commented May 4, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 5, 2018

Now I'm more confused.

I think it's valuable to show the toggled state in the command palette so that there is consistency between menus and command palette about state (just as there is consistency about the enabled state). I think there is value in pushing that behavior down to phosphor.

I have a slight presumptive negative preference for displaying arbitrary icons (other than the indication of toggle status) in the command palette.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 7, 2018

Just to clarify, nobody is talking about adding icons to the commands.
We're only discussing whether phosphor should treat the check marks as an
'icon' or whether the implementation of that checkmark should be done in
JupyterLab's styling.

This is what I'm talking about. The Menu widget doesn't make the distinction between an icon and a check mark, it simply gives you an icon node and appropriate CSS class names for your CSS to style the node how it wants.

It doesn't make much sense from an API perspective to allow check marks, but not icons, especially since such a check mark node will just be abused from CSS to display arbitrary icons.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented May 7, 2018

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 7, 2018

I don't think anyone is thinking about separate clickable checkmarks. The checkmark is just an indicator of the toggled state of the command. Clicking the command toggles the state (presumably).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 7, 2018

especially since such a check mark node will just be abused from CSS to display arbitrary icons.

That would be quite the abuse - are you thinking that people will set the toggled state to always true and use css to display an arbitrary icon?

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 7, 2018

No. I'm saying people will use a dedicated check mark node (in combination with data- attributes) to display arbitrary icons if we dont provide an icon node. So we might as well just give them an icon node, and let them use it for double-duty icons/checks, just like a menu item.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 7, 2018

To be 100% clear about my suggestion:

If we want check marks in command items, then we should just go ahead and add an icon node to the default command palette item renderer. This would make the resulting node very similar in structure to a menu item: <icon><label><shortcut>, where the <icon> node displays either an icon or a check mark, depending on how the user defines their CSS.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 7, 2018

But this would also mean that if they had an icon associated with a command (which I assume could be a common occurrence, since adding a command to a toolbar might automatically use the icon), the icon would show up.

I'm saying that I (probably) don't want the icon rendered in the command palette, regardless if one is specified. The command palette as it is currently implemented stays on the screen, and I think having icons will make it too busy. A menu is only on the screen temporarily, and we assume the user is focusing on the menu when it is open - I think it's okay to have icons in that case. A toolbar is always on the screen, but it is much simpler, with far fewer commands, so I think it's not too busy either. However, I think a command palette would be too busy.

I think you're arguing from an api standpoint - from that standpoint, I agree that having consistency between the menu and command palette makes sense. I'm arguing from a UI perspective, where I think icons would probably make the command palette too busy. My argument is hypothetical - I haven't actually tried it, and I haven't ever heard UI research one way or the other, so take it with a grain of salt.

I would (maybe?) feel different if the command palette was also a temporary floating palette, instead of an on-screen fixture in the UI.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 7, 2018

Ah, but to your point - the CSS determines whether the icon is actually displayed or not, right? In that case, I don't have a strong objection to exposing it at the phosphor level, and instead I move my icon objection to the JLab default theme CSS.

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented May 8, 2018

But this would also mean that if they had an icon associated with a command (which I assume could be a common occurrence, since adding a command to a toolbar might automatically use the icon), the icon would show up.

Yes, and that's already the case with menu items.

Ah, but to your point - the CSS determines whether the icon is actually displayed or not, right?

Precisely. My entire argument is about what should be present at the Phosphor API level, not how you use that functionality in JLab CSS.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented May 8, 2018

Regarding phosphor, my inclination is that the icon/toggled space should be present. Without that, it is easy to have the semantic information in two different renderings of the same command be inconsistent or confusing.

@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants