-
Notifications
You must be signed in to change notification settings - Fork 2.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
[Table] Change header menu icon to chevron and add a bounding box for better affordance #891
Conversation
Agree with the hover/non-hover color, that's certainly a visual bug that we've been wanting to address! 🎉 Regarding the icon change:
Clearly we had trouble with chevrons here in the past, and dot-dot-dot now doesn't seem quite ideal. Do we postpone this matter until we fix truncation and "..." in every cell with oversized content, do we try to find an alternative to "..."? |
I like the idea of changing to the chevron. There are many sorting icons and they can be collapsed into the chevron menu. I think the ... overload is a serious problem so we should bias to changing now, rather than fixing the much more challenging problem of fixing truncation & wrapping 😀 |
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.
Sorry, I'm not comfortable moving forward with the icon change. I prefer "..." overload than caret confusion, as we've seen in the past. In a demo, the former is acceptable while the latter isn't
It's actually worse now though. If you've got a lot of '...'s on the screen, people don't see the ... in the header. So now we're missing it even more than we could have with the caret. '...' is a true overload, whereas the caret is only a perceived overload where some people could get confused with sorting - but our sort icon is actually totally different. |
It isn't about our sort icon, it's about general table sorting patterns outside of Palantir: they use carets. It's super common and if we're breaking out of that, we probably need a different icon, which is why https://pl.ntr/eB exists. Additionally, I'm super against a band-aid influencing other parts of the product. That's how we end up in a failed state like recently. The band-aid here is the way we currently truncate cells with a dot-dot-dot icon. That would mean that we have to change dot-dot-dot usage elsewhere, like column headers? I don't buy this reasoning |
Hmm, this is tricky. Each icon we've considered as an overloaded meaning in a table context:
Google Sheets uses a caret with a background and border. Makes it fairly clear that this is not a sort-related indicator: How about that? |
That's good @cmslewis! I remember we considered a chevron in a button but never implemented it. Let's try that out, it will most likely require some custom styles for the border and such (don't think we can rely on default button styles, may look muddy), I can assist with CSS and @pkwi will make sure it looks 👌 |
will there ever a situation when the table header dropdown menu should render above the header instead of below? |
Ok chatted with Gaby offline and she's on-board with keeping the '...' given that the caret isn't much better (or even worse). I don't feel strongly that caret is better, just that current is not good. |
@michael-yx-wu unlikely, unless your table height is very tiny. In any case, we have smart positioning for popovers and it should work within the table (it may not at the moment) |
Pretty happy with the latest preview, what do y'all think? |
I like it 👍 |
@@ -83,25 +84,45 @@ | |||
height: $interaction-bar-height; | |||
text-align: center; | |||
|
|||
> span { | |||
> [class*="pt-icon"] { |
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 selector seems overly complicated, why not just style .pt-icon
or .pt-icon-standard
(whatever the sizing class is)? same below
We can discuss the hover thing separately, it's not relevant to this PR. @pkwi is it expected that the small chevron is not centered in its 16px bounding box? |
I think if we're changing to make this more discoverable/understandable we could easily update the hover here. |
@tgreenwatts we have gone through the hover story many times, I'm happy to bring you up to speed in person but it's not the topic here. @pkwi quick approval before merging |
I like the gray square as well. It seems to match the default table themes a bit better and doesn't draw too much attention to itself. |
And I think I'd prefer a default minimal button styling. This looks a little too unique, particularly when we add the primary intent on click.
|
I don't think it's "too unique", that would suggest the design system is too rigid IMO. I've already seen reasonable requests for adding visual intents only in button active/hover states and this doesn't seem far from that. |
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.
Regarding the positioning of the icon within its background, I could always try to hack it for different browsers but it's really not worth it. I'm leveraging the icon bounding box directly, it's super simple. The alignment problem most likely comes from the icon font and problems highlighted in this issue: #365
I can always follow up with visual improvements later if necessary, but I think we're good here.
Yea this got merged before the click region was changed, you can click a way far away from the chevron and still open the menu which is weird. @cmslewis is going to update it |
It's like this by design. Small button needs a large click target, else it's a pain to aim. Please leave as is |
We stick to a 30px hit target because a bunch of users can't aim at small icons. This approach has been successful, whereas smaller hit targets have caused problems in the past. It's also a basic usability rule in software design, there's real research on that |
Changes proposed in this pull request:
Quick little thing. In the spirit of aggressively opening PRs and then asking for feedback, I propose that we change the default header-menu icon from
pt-icon-more
(···) topt-icon-chevron-down
(⌄), which looks and feels much more intuitive to me.Also, I found it annoying that the icon looked the same whether you were hovering inside or outside of it within the header cell. This PR shows the icon with
$pt-icon-color
when you hover in the header cell but NOT within the icon, showing$pt-icon-color-hover
only when you mouse over the clickable icon area.Before:
After (Light theme):
After (Dark theme):
For what it's worth, these changes seem consistent with the original mocks for the Table from way back when (although the chevron in the mocks feels too big):
Reviewers should focus on: