ui: remove tooltip underline in sorted table headers#73455
ui: remove tooltip underline in sorted table headers#73455craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
pkg/ui/workspaces/cluster-ui/src/sortedtable/tableHead/tableHead.tsx, line 24 at r1 (raw file):
sortSetting?: SortSetting; firstCellBordered: boolean; noHeaderTooltips?: boolean;
Hmm, any thoughts on whether I should just be adding a field for a class instead, so we can inject custom styling for row headers?
jocrl
left a comment
There was a problem hiding this comment.
Hmm, in my understanding this controls whether the dashed underline is shown or not shown for all table headers, correct? I'll bring up two situations. The second I bring up as more of a learning/thought exercise example, so bear with me that it's long 😅.
- What if (in the above table screenshot), "Indexes" and "Total Reads" don't have tooltips, but the "Last Used (UTC)" column does?
- What are the risk/chances here that this control of the appearance becomes out of sync with whether the header actually has a tooltip? E.g., if there is a tooltip but no underline, or there is an underline but no tooltip. Specifically
a. Is becoming out of sync possible?
b. If so, when would the discrepancy be caught: in development (e.g. typescript/lint/test), in runtime, or would it be silent?
c. How bad are the consequences of getting out-of-sync?
Regarding 1., I think we want to control tooltips on headers on a per-header basis, not just all or nothing for the entire table. The table has the columns prop where you pass in a ColumnDescriptor for each column, and I think that ColumnDescriptor would be a good place to put the tooltip toggle.
Regarding 2a, it seems like there is a possibility of getting out of sync because there are two sources of truth for whether there is a tooltip: how this visual toggle boolean is set, and whether there actually is a tooltip. The best way to not get out of sync would be if there is one source of truth, e.g. the visual class automatically gets applied depending on whether there is a tooltip.
So, how would the table ask whether this column header has an actual tooltip? The contents of the table header, including the tooltip, are passed as this title prop. This makes it difficult for the table to determine whether there is a tooltip rendered somewhere in that. If the type of title passed in is a string you would know that there is no tooltip, but if it's a React component you don't really know what's inside.
As a hypothetical contrast example, if the definition for ColumnDescriptor was something like:
titleText: string;
titleTooltipText?: string;
and the table component made the tooltip, then you could condition the dashed line class based on the presence of the titleTooltipText prop.
But that's hypothetical, and not what we're dealing with 😛. Back to reality, another option for one source of truth would be if the Tooltip component itself applied the dashed line. Problems with this solution are that there are potentially a lot of different tooltip components in use that would have to be changed, and that changing the behavior would likely affect a bunch of other places that are not table headers, and it would be difficult to assess the impact of all the uses. So the dev cost of this solution seems large (and would also be easier to do after tooltips have been unified).
Since that solution seems expensive, I'll go back to
b. If so, when would the discrepancy be caught: in development (e.g. typescript/lint/test), in runtime, or would it be silent?
c. How bad are the consequences of getting out-of-sync?
Regarding b., with the current setup it would be silent; i.e. we wouldn't know unless we look at it an know. Generally the earlier we can find out errors the better, and finding out is better than not finding out.
That said, regarding c., the consequences of getting out of sync are... in my opinion, not that large? Like it's not great, but it's not horrible either. The relatively small impact of an out of sync bug might not be worth more expensive solutions to ensure that it doesn't happen.
So as for what concretely to do about the out of sync problem... I'm not sure 😅. I'm inclined to suggest leaving the possibility that things might get out of sync, and filing improving that as a separate issue that would also be easier to do after the tooltip components are unified.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/sortedtable/tableHead/tableHead.tsx, line 24 at r1 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
Hmm, any thoughts on whether I should just be adding a field for a class instead, so we can inject custom styling for row headers?
Right now custom styling for row headers would be implemented by passing styling on the title prop passed to the column (i.e. pass a react component with styling instead of a string). I think it's better to not introduce another place here for adding custom styling.
783508a to
f836b57
Compare
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/sortedtable/tableHead/tableHead.tsx, line 24 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
Right now custom styling for row headers would be implemented by passing styling on the
titleprop passed to the column (i.e. pass a react component with styling instead of a string). I think it's better to not introduce another place here for adding custom styling.
Sounds good! One problem I noticed is that although you can specify the Tooltip class in the title prop, the actual dashed underline is attached to the surrounding span component, so you can't modify that by passing in styling.
I've updated my code to pass in a new prop for each column instead of the table. Thanks for pointing that out!
lindseyjin
left a comment
There was a problem hiding this comment.
Hmm, I agree that it would be nice if the dashed underline automatically synced with a tooltip. However, because the Tooltip element is currently passed in as part of the title prop styling, it would require a lot more refactoring.
If you think it's necessary, I can open up another issue for this and possibly come back to this later?
Reviewable status:
maryliag
left a comment
There was a problem hiding this comment.
We have an issue for moving the line to the Tooltip component: #68821
Reviewable status:
|
pkg/ui/workspaces/cluster-ui/src/sortedtable/tableHead/tableHead.tsx, line 24 at r1 (raw file): Previously, lindseyjin (Lindsey Jin) wrote…
Ah, I see what you mean. In that case, I would say that
|
|
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx, line 41 at r2 (raw file):
This comment seems a little confusing; my understanding is that "whether the title should have a dashed underline" defaults to True, but the value of How about "Hides the dashed title underline, which represents that a tooltip exists. Shows the underline if not defined." |
Resolves cockroachdb#73280 Previously, all tables headers were expected to have a tooltip; hence they were default styled with a dashed underline. However, we would like to reduce the number of tooltips in the UI, as they can be distracting and take up clutter on the screen. This commit adds in a new configurable column descriptor parameter "hideTitleUnderline" which specifies whether the dashed underline on a column title should be hidden, for titles with no tooltips. This also removes the dashed underline for the Index Stats table, which has no tooltips. Release note (ui change): Add ability to remove dashed underline from sorted table headers for headers with no tooltips. Remove the dashed underline from the Index Stats table headers.
f836b57 to
7b3c41d
Compare
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/sortedtable/sortedtable.tsx, line 41 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
// Whether the title should have a dashed underline, representing that a // tooltip exists. True if not defined. hideTitleUnderline?: boolean;This comment seems a little confusing; my understanding is that "whether the title should have a dashed underline" defaults to True, but the value of
hideTitleUnderlinedefaults to FalseHow about "Hides the dashed title underline, which represents that a tooltip exists. Shows the underline if not defined."
Oops, I originally had the boolean the other way around but forgot to update the comment.
Thanks for catching that, fixed!
pkg/ui/workspaces/cluster-ui/src/sortedtable/tableHead/tableHead.tsx, line 24 at r1 (raw file):
Previously, jocrl (Josephine) wrote…
Ah, I see what you mean. In that case, I would say that
- I think we want all dashed lines indicating tooltips to have the same styling, so it's better to not allow customization on that. That's a way to end up with three different dashed line classes 🙃
- It's easier for people using the table component to specify the boolean, than to have to find the class used for the dashed line
- I'm understanding that there's no current use case for modifying the dashed line style, so let's not build for use cases that don't exist yet. Someone can always modify the code if the need comes up.
Ah I see, that makes sense!
lindseyjin
left a comment
There was a problem hiding this comment.
@maryliag It seems like that issue tackles the same problem (allowing the dashed underline to be removed). In that case, would you recommend updating my code to move the underline to the Tooltip component, or keeping it this way and marking that issue as resolved?
Reviewable status:
jocrl
left a comment
There was a problem hiding this comment.
Reviewable status:
jocrl
left a comment
There was a problem hiding this comment.
edit: I mentioned in the issue that @maryliag linked that this can be pulled out when that deeper fix is made
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Resolves #73280
Previously, all tables headers were expected to have a tooltip; hence
they were default styled with a dashed underline. However, we would like
to reduce the number of tooltips in the UI, as they can be distracting
and take up clutter on the screen.
This commit adds in a new paramter "noHeaderTooltips" which indicates
that the table headers have no tooltips and should have no underline
styling. This also removes the dashed underline for the Index Stats
table, which has no tooltips.
Release note (ui change): Add ability to remove dashed underline from
sorted table headers for headers with no tooltips. Remove the dashed
underline from the Index Stats table headers.