feat(cursor): improve theme styling for crosshair #980

markov00 · 2021-01-15T09:37:40Z

Summary

This PR improved the styling for the crosshair cursor:

the crosshair theme has a more specific crossLine styling object that is used style the horizontal line when using a TooltipType.Crosshairs

interface CrosshairStyle {
  band: FillStyle & Visible;
  line: StrokeStyle & Visible & Partial<StrokeDashArray>;
  crossLine: StrokeStyle & Visible & Partial<StrokeDashArray>;
}

the "vertical" now correctly pick the style from the theme depending on its shape (using the band style when the chart has at least one bar series, using the line style when the chart is composed of line or/and areas) and using the crossLine to style the horizontal crossing line

The cursor implementation was also refactored using SVG instead of DIV elements allowing us to correctly align the cursors to the underlying scale, fixing the current misalignment visible in various cases

Before and after

fix #925

markov00 · 2021-01-15T09:42:00Z

Hey @miukimiu could you please take a look at the default color I've used here for the line cursor?
I think we should update the EUI theme to use the right color for the crosshair.line to have a smooth transition when updating both libraries in kibana
You can test the default color assigned here in the local storybook at http://localhost:9001/?path=/story/stylings--dark-theme and playing with dark mode or light mode and with or without bars

codecov-io · 2021-01-15T09:45:39Z

Codecov Report

Merging #980 (17b85bf) into master (89d4bdb) will decrease coverage by 0.20%.
The diff coverage is 58.53%.

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
- Coverage   71.08%   70.87%   -0.21%     
==========================================
  Files         344      344              
  Lines       10959    10971      +12     
  Branches     2303     2309       +6     
==========================================
- Hits         7790     7776      -14     
- Misses       3155     3181      +26     
  Partials       14       14

Flag	Coverage Δ
unittests	`70.87% <58.53%> (-0.21%)`	⬇️

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

Impacted Files	Coverage Δ
src/utils/themes/dark_theme.ts	`100.00% <ø> (ø)`
src/utils/themes/light_theme.ts	`100.00% <ø> (ø)`
src/utils/themes/theme.ts	`100.00% <ø> (ø)`
...rc/chart_types/xy_chart/renderer/dom/crosshair.tsx	`70.17% <50.00%> (-10.19%)`	⬇️
.../chart_types/xy_chart/crosshair/crosshair_utils.ts	`73.49% <61.53%> (-19.66%)`	⬇️
..._types/xy_chart/state/selectors/get_cursor_band.ts	`93.02% <100.00%> (+0.16%)`	⬆️
..._types/xy_chart/state/selectors/get_cursor_line.ts	`100.00% <100.00%> (ø)`
...s/xy_chart/state/selectors/get_tooltip_position.ts	`75.00% <0.00%> (-18.75%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89d4bdb...7dd42b2. Read the comment docs.

nickofthyme

LGTM, tested locally. This looks so much nicer and cleaner using the svg instead 👍

src/chart_types/xy_chart/crosshair/crosshair_utils.ts

src/chart_types/xy_chart/renderer/dom/_crosshair.scss

rshen91

LGTM!

elizabetdev

LGTM! 🎉

I tested locally and just added a few suggestions. I can update the EUI theme once the PR is merged.

elizabetdev · 2021-01-15T17:00:18Z

src/utils/themes/light_theme.ts

+      strokeWidth: 1,
+      visible: true,
+    },
+    crossLine: {
      stroke: '#777',


The line.stroke and crossLine.stroke could use the same color. I think the #98A2B3 has more contrast.

Suggested change

stroke: '#777',

stroke: '#98A2B3',

Just to note: these styles are only default to elastic charts. If you want this change reflected in kibana you'd need to update the eui charts theme.

https://github.com/elastic/eui/blob/d16d296ae4d06a6337a4a2534f4d3bfacff932ea/src/themes/charts/themes.ts#L172-L181

done in #5e8f506

elizabetdev · 2021-01-15T17:03:08Z

src/utils/themes/dark_theme.ts

@@ -176,6 +176,11 @@ export const DARK_THEME: Theme = {
      visible: true,
    },
    line: {
+      stroke: '#535966',


The line.stroke and crossLine.stroke could use the same color. And #999 has more contract on darker backgrounds:

Suggested change

stroke: '#535966',

stroke: '#999',

done in #5e8f506

# [24.5.0](v24.4.0...v24.5.0) (2021-01-30) ### Bug Fixes * add theme min radius to point shape ([#996](#996)) ([eb37175](eb37175)) * align tooltip z-index to EUI tooltip z-index ([#931](#931)) ([ffd626b](ffd626b)) * chart state and series functions cleanup ([#989](#989)) ([944ac6c](944ac6c)) * create unique ids for dot icons ([#971](#971)) ([e1ce768](e1ce768)) * external tooltip legend extra value sync ([#993](#993)) ([13ad05a](13ad05a)) * **legend:** disable focus and keyboard navigation for legend in partition ch… ([#952](#952)) ([03bd2f7](03bd2f7)) * **legend:** hierarchical legend order should follow the tree paths ([#947](#947)) ([f9218ad](f9218ad)), closes [#944](#944) * **legend:** remove ids for circles ([#973](#973)) ([b3f4f90](b3f4f90)) ### Features * **cursor:** improve theme styling for crosshair ([#980](#980)) ([6c4dafd](6c4dafd)) * **legend:** display pie chart legend extra ([#939](#939)) ([d14de01](d14de01)) * **legend:** add keyboard navigation ([#880](#880)) ([87c227d](87c227d)) * **partition:** Flame and icicle chart ([#965](#965)) ([3df73d0](3df73d0)) * **partition:** legend hover options ([#978](#978)) ([f810d94](f810d94)) * **xy:** support multiple point shapes on line, area and bubble charts ([#988](#988)) ([1392b7d](1392b7d))

markov00 · 2021-01-30T15:42:19Z

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

markov00 added 3 commits January 15, 2021 10:35

docs: update API report

b7fd2a0

refactor: add crossLine style

47c1303

feat(cursor): improve cursor styling

8eb88da

markov00 added enhancement New feature or request :interactions Interactions related issue labels Jan 15, 2021

markov00 requested review from nickofthyme, rshen91 and elizabetdev January 15, 2021 09:37

Merge branch 'master' into 2021_01_14-fix_crosshair

f859c24

nickofthyme approved these changes Jan 15, 2021

View reviewed changes

src/chart_types/xy_chart/crosshair/crosshair_utils.ts Show resolved Hide resolved

src/chart_types/xy_chart/renderer/dom/_crosshair.scss Show resolved Hide resolved

rshen91 approved these changes Jan 15, 2021

View reviewed changes

elizabetdev reviewed Jan 15, 2021

View reviewed changes

markov00 added 3 commits January 18, 2021 18:58

fix: band on ordinal areas/lines

5e8f506

Merge branch 'master' into 2021_01_14-fix_crosshair

97b0f5b

test: update screenshot for missing stories

7dd42b2

markov00 merged commit 6c4dafd into elastic:master Jan 19, 2021

markov00 deleted the 2021_01_14-fix_crosshair branch January 19, 2021 13:06

markov00 added the released Issue released publicly label Jan 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cursor): improve theme styling for crosshair #980

feat(cursor): improve theme styling for crosshair #980

markov00 commented Jan 15, 2021

markov00 commented Jan 15, 2021

codecov-io commented Jan 15, 2021 •

edited

Loading

nickofthyme left a comment

rshen91 left a comment

elizabetdev left a comment

elizabetdev Jan 15, 2021

nickofthyme Jan 15, 2021

markov00 Jan 19, 2021

elizabetdev Jan 15, 2021

markov00 Jan 19, 2021

markov00 commented Jan 30, 2021

feat(cursor): improve theme styling for crosshair #980

feat(cursor): improve theme styling for crosshair #980

Conversation

markov00 commented Jan 15, 2021

Summary

markov00 commented Jan 15, 2021

codecov-io commented Jan 15, 2021 • edited Loading

Codecov Report

nickofthyme left a comment

Choose a reason for hiding this comment

rshen91 left a comment

Choose a reason for hiding this comment

elizabetdev left a comment

Choose a reason for hiding this comment

elizabetdev Jan 15, 2021

Choose a reason for hiding this comment

nickofthyme Jan 15, 2021

Choose a reason for hiding this comment

markov00 Jan 19, 2021

Choose a reason for hiding this comment

elizabetdev Jan 15, 2021

Choose a reason for hiding this comment

markov00 Jan 19, 2021

Choose a reason for hiding this comment

markov00 commented Jan 30, 2021

codecov-io commented Jan 15, 2021 •

edited

Loading