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

chore: legend gap removal #968

Merged
merged 7 commits into from
Jan 14, 2021
Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Jan 12, 2021

Summary

Closes #944
More important for the upcoming hierarchical layout hover strategy PR and the flame/icicle chart than for existing charts

Before, with gaps, see how the activation is temporarily lost between rows, with all colors flashing back:
Screen Recording 2021-01-14 at 03 06 PM

After, no gaps, no flashing:
no gaps

It shifts images up by 1px or 2px, which seems OK, as it's important that padding-top and padding-bottom equal (text is in the vertical middle of the mouse capture zone)

Checklist

Delete any items that are not applicable to this PR.

  • This was checked for cross-browser compatibility
  • Proper documentation or storybook story was added for features that require explanation or tutorials

@monfera monfera added :legend Legend related issue :i18n i18n related issue labels Jan 12, 2021
@monfera monfera added the :accessibility Accessibility related issue label Jan 12, 2021
@monfera monfera changed the title Legend gap removal chore: legend gap removal Jan 12, 2021
@codecov-io
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

Merging #968 (ccffc95) into master (3df73d0) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   71.08%   71.16%   +0.07%     
==========================================
  Files         344      360      +16     
  Lines       10959    10587     -372     
  Branches     2393     2161     -232     
==========================================
- Hits         7790     7534     -256     
+ Misses       3155     2964     -191     
- Partials       14       89      +75     
Flag Coverage Δ
unittests ?

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

Impacted Files Coverage Δ
src/components/legend/legend_item.tsx 91.89% <ø> (-0.42%) ⬇️
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/utils/data_generators/data_generator.ts 45.45% <0.00%> (-9.10%) ⬇️
...t/state/selectors/compute_small_multiple_scales.ts 86.95% <0.00%> (-8.70%) ⬇️
src/state/reducers/interactions.ts 68.42% <0.00%> (-5.05%) ⬇️
...state/selectors/get_internal_is_tooltip_visible.ts 75.00% <0.00%> (-5.00%) ⬇️
... and 201 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 3df73d0...ccffc95. Read the comment docs.

@nickofthyme
Copy link
Collaborator

nickofthyme commented Jan 12, 2021

I think this was working at one point, not 100% sure. This was discussed at length here #749 (comment) when implementing the legend actions.

I don't think this is the right solution for this as it is because it changes the aesthetic of the legend. That said, I 100% agree this needs to be corrected.

@monfera
Copy link
Contributor Author

monfera commented Jan 12, 2021

Thanks @nickofthyme this PR doesn't change aesthetics perceptibly, except a 1-2px shift of the entire legends block upwards, but maybe this is what you referred to (CI detects it) - so it's great to go ahead as we discussed on our call

@nickofthyme
Copy link
Collaborator

@monfera Checkout this commit d9722b6. I am sorry to force push to your branch but a made a copy just in case.

The trick is to add half of the padding to the .echLegendItem element and the other half to a .background element that is absolutely positioned behind the legend item. This way the look is the same but the interaction is smooth across all legend items. Now moving the cursor vertically over the legend items will always show an actively highlighted series.

Screen Recording 2021-01-12 at 04 35 PM

@nickofthyme nickofthyme removed their request for review January 12, 2021 23:11
@monfera
Copy link
Contributor Author

monfera commented Jan 13, 2021

@nickofthyme looks great, I agree with the push -f here too. Could you look into the two CI balks?

@nickofthyme
Copy link
Collaborator

nickofthyme commented Jan 13, 2021

It looks like the legend overall was slightly shifted up, not sure why, but other than that is the same ui.

Screen Recording 2021-01-13 at 12 33 PM

@nickofthyme
Copy link
Collaborator

@miukimiu Could you take a look at these changes? Should be the same UI overall.

@monfera
Copy link
Contributor Author

monfera commented Jan 14, 2021

@nickofthyme how should the still remaining CI issue be solved? Is it a question of regenerating some image, or something deeper? Should we, and/or @miukimiu look into removing the visual difference you show above? My slant is, seeing this merged soon as it's a small code change yet good usability improvement, and I'm not concerned about snapshot changes of a couple of pixels of legend position

@nickofthyme
Copy link
Collaborator

@monfera I'll look into the small px shift.

@nickofthyme nickofthyme merged commit 9569ccb into elastic:master Jan 14, 2021
@markov00
Copy link
Member

🎉 This PR is included in version 24.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:accessibility Accessibility related issue :i18n i18n related issue :legend Legend related issue released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[minor] larger and uniform legend capture zones
4 participants