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

feat(ui): Use tabular figures in tables and charts #28871

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

vuluongj20
Copy link
Member

@vuluongj20 vuluongj20 commented Sep 27, 2021

The CSS style font-variant-numeric: tabular-nums; turns on tabular figures - monospaced substitutes for numeric characters in Rubik, ideal for tables and charts.

Screen Shot 2021-09-27 at 1 11 35 PM

This commits adds the tabular-nums CSS style to components across the app.

We shouldn't use tabular figures globally (i.e. we shouldn't add tabular-nums to body {} or table {}). They are only appropriate for certain columns and text spans. For example, in the table row below, we only need tabular figures in the numeric columns on the right. The transaction column on the left contains both letters and numbers, and so would look better with the default proportional figures.

Screen Shot 2021-09-27 at 1 15 59 PM


Here's a list of the components/elements that have been updated to use tabular figures:
(You can spot tabular figures by looking at the 1s, which have a horizontal slab (line) at the bottom – see the difference in the image above.)

  • Numeric columns in tables
    Screen Shot 2021-09-27 at 1 22 24 PM

  • Chart axis labels

Screen Shot 2021-09-27 at 1 23 13 PM

  • Chart tooltips

Screen Shot 2021-09-27 at 1 28 13 PM

  • Percentages in tag distribution meters

Screen Shot 2021-09-27 at 1 25 16 PM

  • Release names (useful for comparing releases in SemVer)

Screen Shot 2021-09-27 at 1 30 23 PM

The CSS style `font-variant-numeric: tabular-nums;` turns on tabular figures - monospaced substitutes for numeric characters in Rubik, ideal for tables and charts.
@vuluongj20 vuluongj20 requested a review from a team as a code owner September 27, 2021 20:32
@vuluongj20 vuluongj20 requested review from a team September 27, 2021 20:32
@vuluongj20 vuluongj20 requested review from a team as code owners September 27, 2021 20:32
@vuluongj20 vuluongj20 requested a review from a team September 27, 2021 20:32
@@ -11,7 +11,7 @@ export default function Grid(props: EChartOption.Grid = {}): EChartOption.Grid {
top: 20,
bottom: 20,
// This should allow for sufficient space for Y-axis labels
left: '0%',
left: 4,
Copy link
Member Author

@vuluongj20 vuluongj20 Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a workaround to prevent y-axis overflows.

Screen Shot 2021-09-27 at 12 46 44 PM

It looks like ECharts calculates the y-axis width by pre-rendering the axis and getting the pre-rendered width. This becomes a problem because there is no explicit way to tell ECharts to use tabular figures in axes (instead, we have to add a special CSS style to the chart container), so it will pre-render axes with the default proportional figures. Numbers with proportional figures are always more narrow than those with tabular figures. As a result, the calculated y-axis width is more narrow than it should be.

I'm not sure how we can cleanly fix this. The workaround I found is to add a left offset to the grid, thus providing more space for the y-axis. From my testing, an offset of 4 should be enough for most use cases.

Screen Shot 2021-09-27 at 1 51 13 PM

@@ -270,7 +270,7 @@ class WidgetCardChart extends React.Component<WidgetCardChartProps> {
const axisField = widget.queries[0]?.fields?.[0] ?? 'count()';
const chartOptions = {
grid: {
left: 0,
left: 4,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround for y-axis width issue. See comment above ⬆️

@@ -194,7 +194,7 @@ class MiniBarChart extends React.Component<Props> {
// default size.
top: labelYAxisExtents ? 6 : 0,
bottom: markers || labelYAxisExtents ? 4 : 0,
left: markers ? 4 : 0,
left: markers ? 8 : 4,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround for y-axis width issue. See comment above ⬆️

@@ -614,6 +614,7 @@ const TimeAxis = styled('div')`
color: ${p => p.theme.gray300};
font-size: 10px;
font-weight: 500;
font-variant-numeric: tabular-nums;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vuluongj20 Can you apply this to the <TickText/> component in this same file (just below)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And <DurationGuideBox/>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm both of these are nested inside <TimeAxis/>, and should inherit the style from it:

Screen Shot 2021-09-27 at 3 45 18 PM

Did you just want me to be more specific (i.e. apply tabular-nums to <TickText/> and <DurationGuideBox/> instead of <TimeAxis/> as the shared parents)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vuluongj20 ah i see. looks good to me!

@vuluongj20 vuluongj20 merged commit 90f753b into master Sep 27, 2021
@vuluongj20 vuluongj20 deleted the vuluong/tabular-figures branch September 27, 2021 23:10
vuluongj20 added a commit that referenced this pull request Sep 30, 2021
* Use tabular figures in tables and charts

The CSS style `font-variant-numeric: tabular-nums;` turns on tabular figures - monospaced substitutes for numeric characters in Rubik, ideal for tables and charts. This commits adds the `tabular-nums` CSS style to components across the app.
@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants