-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(web-analytics): Improve clickability discovery of table rows #18220
feat(web-analytics): Improve clickability discovery of table rows #18220
Conversation
6d4a812
to
5d565bf
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
} | ||
return { | ||
onClick: () => onClick(breakdownValue), | ||
className: 'hover:underline cursor-pointer hover:bg-mark', |
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.
Patterns are really important in UX ("don't make me think") and here we're introducing a one we don't have yet (clickable table row), so let's take a moment to consider how to go about this.
For context, we've purposefully stayed away from clickable rows before in favor of the link-name, because there are two downsides: 1. they don't work well for rows which have any other actions, 2. they're not discoverable without hover (there's no expectation of clickability, as most tables don't have clickable rows).
Hence I don't think it's a given that this change will increase discoverability. But we can try it out.
In trying this out, let's make sure that the pattern lives in the table component. Basically I suggest that we don't pass these classes here, but instead if a row has onClick
, it gets these in TableRow.tsx
.
As for the specific styling, let's not use bg-mark
, since it's already used in tables in a different context – it means that the row is statically highlighted. For interactivity bg-primary-highlight
might be better.
It would also be nice if only the first column was underlined, since all the other ones are just metadata not relevant to the on-click action, but that's more of a nice to have.
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.
The problem with just having the link-name clickable (like I had before) is that in this case, sometimes the link name is a path. If you have a clickable e.g. user id, it's pretty obvious what to expect - clicking it is going to tell me more information about that user. If I have a clickable path, there are 2 potentially obvious things to expect - more info or open that link. Making the row clickable, and whole row highlighted, is to nudge the expectation away from opening the link.
Very happy to move the code for this into the table component - will do that now. Edit: done
50e4176
to
7ffe914
Compare
7ffe914
to
a658928
Compare
Problem
From this recording https://app.posthog.com/insights/ciGH0pTP#sessionRecordingId=018b66e8-5040-728e-98aa-7c9cd56b5c3e some of the UI elements are not obvious how to use.
I've made the entire row of tables more obviously clickable, and I've copied the UI paradigm from our LemonTabs component into a new WebTabs component, that works similarly but has a changable title
Changes
Untitled.mov
How did you test this code?
Still behind FF, ran it manually