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

[Table] Change header menu icon to chevron and add a bounding box for better affordance #891

Merged
merged 4 commits into from Mar 29, 2017

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Mar 27, 2017

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 (···) to pt-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:
2017-03-27 14 41 22

After (Light theme):
2017-03-27 14 29 16

After (Dark theme):
2017-03-27 14 30 31

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):
image

Reviewers should focus on:

  • I think the second piece (the hover/non-hover color) should merge regardless—do you agree?
  • Do you like the icon change?

@cmslewis cmslewis changed the title [Table] Change header-menu icon to 'caret-down', and mute the default color too [Table] Change header-menu icon to 'chevron-down', and mute the default color too Mar 27, 2017
@blueprint-bot
Copy link

Show more muted icon color when not directly hovering on th-menu

Preview: docs | table
Coverage: core | datetime

@llorca
Copy link
Contributor

llorca commented Mar 28, 2017

Agree with the hover/non-hover color, that's certainly a visual bug that we've been wanting to address! 🎉

Regarding the icon change:

  • I've heard concerns about too many "..." in the table. It's because of truncation which adds an ellipsis popover when the content is too large for the cell, but the way we handle truncation now is a band-aid IMO -- we did what we could with the current tech limitations. For instance, Excel and Spreadsheet handle it better and don't need additional "..."
  • The designs are outdated, we actually recently changed the icon from chevron to dot-dot-dot for two reasons, first it was too easily confused with the icon for the action of sorting a column, which by standard is a chevron/caret up/down; and second the dot-dot-dot simply seemed like a better generic icon for representing a list of actions or menu (it's also an accepted pattern in other software platforms)

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 "..."?

adidahiya
adidahiya previously approved these changes Mar 28, 2017
@tgreenwatts
Copy link
Contributor

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 😀

Copy link
Contributor

@llorca llorca left a 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

@tgreenwatts
Copy link
Contributor

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.

@llorca
Copy link
Contributor

llorca commented Mar 28, 2017

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

@cmslewis
Copy link
Contributor Author

Hmm, this is tricky. Each icon we've considered as an overloaded meaning in a table context:

  • ··· - also means "there is more text than fits in view"
  • / - (in a header cell) also means "this column is or can be sorted"
  • - (in a header cell) frankly haven't seen this as often as a caret for sorting purposes, but I guess it could reasonably mean "this column is or can be sorted"

Google Sheets uses a caret with a background and border. Makes it fairly clear that this is not a sort-related indicator:

image

How about that?

@llorca
Copy link
Contributor

llorca commented Mar 28, 2017

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 👌

@cmslewis
Copy link
Contributor Author

Sounds good. @llorca @pkwi - feel free to push to this branch with those style changes.

@michael-yx-wu
Copy link
Contributor

will there ever a situation when the table header dropdown menu should render above the header instead of below?

@tgreenwatts
Copy link
Contributor

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.

@llorca
Copy link
Contributor

llorca commented Mar 28, 2017

@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)

Edit: it seems to be working
image

@adidahiya adidahiya dismissed their stale review March 28, 2017 21:26

deferring to antoine's design feedback

@blueprint-bot
Copy link

Add styles for action icon in column header

Preview: docs | table
Coverage: core | datetime

@llorca
Copy link
Contributor

llorca commented Mar 28, 2017

Pretty happy with the latest preview, what do y'all think?

@adidahiya
Copy link
Contributor

I like it 👍

@@ -83,25 +84,45 @@
height: $interaction-bar-height;
text-align: center;

> span {
> [class*="pt-icon"] {
Copy link
Contributor

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

@adidahiya adidahiya changed the title [Table] Change header-menu icon to 'chevron-down', and mute the default color too [Table] Change header menu icon to chevron and add a bounding box for better affordance Mar 29, 2017
@tgreenwatts
Copy link
Contributor

tgreenwatts commented Mar 29, 2017

Column D doesn't work
image

I don't like that it's not shown until hover - is that always the case? Now I see a stinking ... (the non-clickable kind) and then surprise I have a thing on hover.

Am I crazy for thinking this visually looks a little weird? Less polished than the rest of blueprint ? I don't know.
image
[edit] Confirmed with others that the arrow is off-center, and it just looks not great somehow...

@llorca
Copy link
Contributor

llorca commented Mar 29, 2017

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?

@llorca
Copy link
Contributor

llorca commented Mar 29, 2017

Other visual option:
screen shot 2017-03-28 at 5 53 43 pm

@tgreenwatts
Copy link
Contributor

I think if we're changing to make this more discoverable/understandable we could easily update the hover here.
If that's expected to be offcenter then that seems not good.

@llorca
Copy link
Contributor

llorca commented Mar 29, 2017

@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

@pkwi
Copy link
Contributor

pkwi commented Mar 29, 2017

The positioning seems fine.

screen shot 2017-03-29 at 12 04 30 pm

Yeah, I like the chevron within a small grey square. Not too heavy but distinguishable from the sorting icon

@michael-yx-wu
Copy link
Contributor

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.

@cmslewis
Copy link
Contributor Author

cmslewis commented Mar 29, 2017

  1. @pkwi @llorca I'm seeing it off-center on OS X Chrome:

image

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.

  1. @tgreenwatts @llorca not to continue an off-topic discussion, but I like having the icon hide until you hover on the column header. Otherwise you have a bunch of irrelevant visual elements competing for your attention.

@adidahiya
Copy link
Contributor

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.

Copy link
Contributor

@llorca llorca left a 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.

@blueprint-bot
Copy link

Better selector for action icon in column header

Preview: docs | table
Coverage: core | datetime

@adidahiya adidahiya merged commit 50bb8df into master Mar 29, 2017
@adidahiya adidahiya deleted the cl/table-caret-icon branch March 29, 2017 20:19
@tgreenwatts
Copy link
Contributor

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

@llorca
Copy link
Contributor

llorca commented Mar 29, 2017

It's like this by design. Small button needs a large click target, else it's a pain to aim. Please leave as is

@tgreenwatts
Copy link
Contributor

It's more than double the size, I think it's too big. I can click all the way to the left of the 'a' here
image

@llorca
Copy link
Contributor

llorca commented Mar 29, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants