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

fix(timeline): Fix yaxis label color in dark mode #3698

Conversation

joshuarrrr
Copy link
Member

@joshuarrrr joshuarrrr commented Mar 27, 2023

Description

  • Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
  • Update CSS styles of yaxis label to match chart title
  • Update HTML rendering to support custom color overrides via inline styles

Before:

Screen.Recording.2023-03-27.at.6.39.23.PM.mov

After:

Screen.Recording.2023-03-29.at.4.27.19.PM.mov

Explanation

There are a couple things going on here. The timeline vis was previously always setting the property axisLabelUseCanvas to true for the yAxis. Within flot.js, which is the legacy charting library used by timeline, that property causes the yAxis label to be rendered as part of the <canvas> element, which means it doesn't pick up any of the default CSS styles. In contrast, other chart text (such as the chart title) are rendered via HTML, which means they get the appropriate EUI text color applied, regardless of whether the dark or light theme is selected.

As far as I know, we don't currently have any good mechanisms to access theme colors within JS/TS files. If we did, it would be simple to just update the default color to just pull from the current theme text color:

Instead, the fix was to switch the rendering of the yAxis label to HTML, which flot already supports. Unfortunately, the HTML renderers (HtmlAxisLabel and CssTransformAxisLabel) were missing support for custom colors, which timeline visualizations support as part of the yaxis expression. So I added custom color support to those renderers to be backwards compatible with timeline expressions that had set a custom color for that label. Finally, I styled the axis label to match the chart title.

Issues Resolved

Fixes #2250

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

- Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
- Update CSS styles of yaxis label to match chart title
- Update HTML rendering to support custom color overrides via inline styles

Signed-off-by: Josh Romero <rmerqg@amazon.com>
@joshuarrrr joshuarrrr requested a review from a team as a code owner March 27, 2023 17:45
Fixes opensearch-project#2250

Signed-off-by: Josh Romero <rmerqg@amazon.com>
@joshuarrrr joshuarrrr self-assigned this Mar 27, 2023
Signed-off-by: Josh Romero <rmerqg@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 30, 2023

Codecov Report

Merging #3698 (348f3b8) into main (7efc231) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3698      +/-   ##
==========================================
- Coverage   66.44%   66.44%   -0.01%     
==========================================
  Files        3209     3209              
  Lines       61633    61633              
  Branches     9507     9506       -1     
==========================================
- Hits        40955    40954       -1     
- Misses      18398    18400       +2     
+ Partials     2280     2279       -1     
Flag Coverage Δ
Linux 66.39% <100.00%> (-0.01%) ⬇️
Windows 66.39% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...vis_type_timeline/server/series_functions/yaxis.js 71.42% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

The change might be small, but i can tell that you did your homework on this one. Also thanks for the helpful PR description. made this review a lot easier :)

@ananzh
Copy link
Member

ananzh commented Mar 31, 2023

The change might be small, but i can tell that you did your homework on this one. Also thanks for the helpful PR description. made this review a lot easier :)

+1

@joshuarrrr joshuarrrr merged commit 868c822 into opensearch-project:main Mar 31, 2023
@joshuarrrr joshuarrrr deleted the fix/timeline-yaxis-label-darkmode branch March 31, 2023 16:47
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 31, 2023
- Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
- Update CSS styles of yaxis label to match chart title
- Update HTML rendering to support custom color overrides via inline styles

Fixes #2250

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit 868c822)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
abbyhu2000 pushed a commit that referenced this pull request Mar 31, 2023
- Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
- Update CSS styles of yaxis label to match chart title
- Update HTML rendering to support custom color overrides via inline styles

Fixes #2250

Signed-off-by: Josh Romero <rmerqg@amazon.com>
(cherry picked from commit 868c822)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
vagimeli pushed a commit to vagimeli/OpenSearch-Dashboards that referenced this pull request Apr 4, 2023
…#3698)

- Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
- Update CSS styles of yaxis label to match chart title
- Update HTML rendering to support custom color overrides via inline styles

Fixes opensearch-project#2250

Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: vagimeli <vagimeli@amazon.com>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…#3698)

- Render label as HTML instead of in the Canvas so that it picks up text color from CSS theme
- Update CSS styles of yaxis label to match chart title
- Update HTML rendering to support custom color overrides via inline styles

Fixes opensearch-project#2250

Signed-off-by: Josh Romero <rmerqg@amazon.com>
Signed-off-by: David Sinclair <david@sinclair.tech>
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.

[BUG] Timeline Y Axis labels are the wrong colour in dark mode making them very difficult to see
4 participants