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

Legend / color picker overflow #30960

Merged
merged 31 commits into from Feb 19, 2019

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Feb 13, 2019

Summary

Fixes #17488

Depends on #30478

This fixes some layouting problems with the legend/ color picker:

  • For horizontal legends it is displayed as overlay instead of inline
  • Harmonize size and margin of color dots
  • Move it down a bit to not overlap with the focus outline
  • Fix jumping of selected legend item

This doesn't fix the problem with multi-line horizontal legends pushing down the chart @ppisljar noted in an old unmerged PR (#17489) for this issue but I think we should fix that one in a separate PR because it seems to be caused by the visualization itself not correctly updating its bounds.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@flash1293 flash1293 requested a review from a team as a code owner February 13, 2019 15:53
@flash1293 flash1293 changed the title [wip] [skip-ci] Legend / color picker overflow Legend / color picker overflow Feb 13, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

/**
* 1. Position the .visLegend__valueDetails absolutely against the legend item
* 2. Make sure the .visLegend__valueDetails is visible outside the list bounds
* 3. Make sure the currently selected item is top most in z level
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these comments removed?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Oh yeah this is much better, thank you. Once those comments go back in, I think it's ready for CI. I just had one more suggestion.


.visLib--legend-bottom & {
bottom: $visLegendLineHeight + $euiSizeXS;
margin-bottom: $euiSizeXS;
Copy link
Contributor

Choose a reason for hiding this comment

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

To clean these two lines you could just add that margin bottom amount to the bottom so it's:

bottom: $visLegendLineHeight + $euiSizeS;

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member

download

  1. when legend is long and spans multiple lines the chart overflows the axes

  2. when opening a legend its not possible to close it anymore (dashboard)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

nevermind the 2). figured i can close it by clicking on the same item again ... but took me some time, so we might want to improve experience around that by hiding it on clicking outside ?

long legends need to be fixed.

@flash1293
Copy link
Contributor Author

flash1293 commented Feb 18, 2019

@ppisljar

when legend is long and spans multiple lines the chart overflows the axes

I mentioned that in the PR description, I will fix this is a separate PR since it is another bug (not related to the overlays itself, happens even if no overlay is expanded) which seems to be caused by the chart not correctly calculating its bounds (perhaps something about the render order?).

figured i can close it by clicking on the same item again ... but took me some time, so we might want to improve experience around that by hiding it on clicking outside?

+1, makes sense to me, what do you think @cchaos ?

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

sorry, missed that. apart from that its a LGTM

@flash1293
Copy link
Contributor Author

@ppisljar @cchaos I looked into the code and it looks like the "on click outside" thing is a bit out of scope for this bug fix PR. Since the overlay will soon(-ish) be replaced by a far better EUI solution I don't think we should add this here and now IMHO.

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2019

I think it's fine to figure that out later or just wait for the new charts

@flash1293 flash1293 merged commit b23e087 into elastic:master Feb 19, 2019
@flash1293 flash1293 deleted the 17488/legend-colorpicker-overflow branch February 19, 2019 16:14
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 19, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 19, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 19, 2019
flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 20, 2019
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] Legend Color Picker Pushes Visualizations down
4 participants