Skip to content

style(crosshair): add z-index to crosshair line#1021

Merged
nickofthyme merged 6 commits intoelastic:masterfrom
elizabetdev:crosshair-line-zindex
Feb 10, 2021
Merged

style(crosshair): add z-index to crosshair line#1021
nickofthyme merged 6 commits intoelastic:masterfrom
elizabetdev:crosshair-line-zindex

Conversation

@elizabetdev
Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev commented Feb 9, 2021

Summary

This PR adds a z-index to the crosshair line.

While working on elastic/eui#4503, I found that the crosslines hide behind the vertical grid lines.

Before

When the vertical grid lines are visible, the crosshairs stay behind the grid lines:

Screen.Recording.2021-02-09.at.01.46.PM.mov

After

With the z-index the crosshairs are now visible when we're on top of a vertical grid line:

Screen.Recording.2021-02-09.at.01.50.PM.mov

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@nickofthyme nickofthyme requested a review from markov00 February 9, 2021 20:33
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1021 (c5b750d) into master (9694797) will decrease coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1021      +/-   ##
==========================================
- Coverage   72.50%   72.48%   -0.03%     
==========================================
  Files         363      379      +16     
  Lines       11167    10789     -378     
  Branches     2431     2205     -226     
==========================================
- Hits         8097     7820     -277     
+ Misses       3056     2866     -190     
- Partials       14      103      +89     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
...rc/chart_types/xy_chart/renderer/dom/crosshair.tsx 71.42% <50.00%> (+1.25%) ⬆️
src/state/selectors/get_legend_items_labels.ts 50.00% <0.00%> (-50.00%) ⬇️
src/utils/dimensions.ts 71.42% <0.00%> (-28.58%) ⬇️
src/components/no_results.tsx 50.00% <0.00%> (-25.00%) ⬇️
src/chart_types/xy_chart/utils/panel_utils.ts 72.72% <0.00%> (-19.59%) ⬇️
src/components/error_boundary/errors.ts 50.00% <0.00%> (-16.67%) ⬇️
src/chart_types/xy_chart/rendering/point_style.ts 87.50% <0.00%> (-12.50%) ⬇️
...t/state/selectors/compute_small_multiple_scales.ts 87.50% <0.00%> (-12.50%) ⬇️
...rt/renderer/dom/annotations/annotation_tooltip.tsx 79.31% <0.00%> (-10.35%) ⬇️
src/utils/data_generators/data_generator.ts 45.45% <0.00%> (-9.10%) ⬇️
... and 207 more

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 9694797...c5b750d. Read the comment docs.

Copy link
Copy Markdown
Collaborator

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

All good for me if @miukimiu is ok having the cursor line above the point circles/shapes.
I think it's ok having the crosshair line above the grid lines, but not sure about above all the chart elements

@elizabetdev
Copy link
Copy Markdown
Contributor Author

I tested and it looks good to me the crosshairs above points and circles.

I did a quick research and most of the charts I found, have the crosshair above circles. Highcharts seems to be an expectation.

@nickofthyme nickofthyme enabled auto-merge (squash) February 10, 2021 14:56
@nickofthyme nickofthyme merged commit 9f9e5c2 into elastic:master Feb 10, 2021
@markov00
Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 24.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released Issue released publicly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants