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.

Sign up for GitHub

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

Jump to bottom

[Lens] XY chart -long legend overflows chart in editor Feature:Lens #70702

Merged
merged 6 commits into from
Jul 14, 2020
Merged

[Lens] XY chart -long legend overflows chart in editor Feature:Lens #70702

merged 6 commits into from
Jul 14, 2020

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Jul 3, 2020 •
edited
Loading

Summary

Fixes #69796

@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.9.0 labels Jul 3, 2020
@mbondyra mbondyra requested a review from a team July 3, 2020 11:20
@mbondyra mbondyra requested a review from a team as a code owner July 3, 2020 11:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor

#69820 is merged now but there are some conflicts. Could you check them out?

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@mbondyra mbondyra closed this Jul 6, 2020
@mbondyra mbondyra deleted the lens/legend_stretch branch July 6, 2020 07:38
@mbondyra mbondyra restored the lens/legend_stretch branch July 6, 2020 07:40
@mbondyra mbondyra reopened this Jul 6, 2020
flash1293
flash1293 approved these changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested in Firefox, LGTM.

I noticed another problem (just in FF):
Screenshot 2020-07-06 at 09 50 44

There is no bottom padding for the suggestion bar. Can be fixed separately though

@mbondyra mbondyra requested a review from cchaos July 6, 2020 07:58
@mbondyra
Copy link
Contributor Author

mbondyra commented Jul 6, 2020

@elasticmachine merge upstream

@flash1293 flash1293 mentioned this pull request Jul 6, 2020
Closed
@wylieconlon
Copy link
Contributor

I see the tag to skip release notes, is this because the bug was not released in 7.8?

@mbondyra
Copy link
Contributor Author

mbondyra commented Jul 6, 2020

@wylieconlon yes, it was introduced with the new chart settings layout (the upper panel)

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

cchaos
cchaos reviewed Jul 13, 2020
View reviewed changes
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.

Pulled down and tested this. Seems to have fixed the original issue. Just need to change the className to adhere to BEM naming conventions.

One annoying rendering issue I'm seeing is that the scrollbar for the legend flashes a bunch when resizing the browser. Any thoughts on why this might be happening?
Screen Recording 2020-07-13 at 05 35 29 PM

...ns/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel_wrapper.tsx Outdated Show resolved Hide resolved
@mbondyra @cchaos
Apply suggestions from code review
fb2d296 
bem convention

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@cchaos About the flashing scroll bar - I think this happens because the elastic-chart is not rerendering fast enough - not sure whether there is a good way to prevent it.

cchaos
cchaos approved these changes Jul 14, 2020
View reviewed changes
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.

👍 LGTM. We should follow up on the flashing scrollbar in another PR because it doesn't happen in the Charts Storybook, so it might be an implementation detail.

@mbondyra mbondyra merged commit cdbe12f into elastic:master Jul 14, 2020
@mbondyra mbondyra mentioned this pull request Jul 14, 2020
Merged
mbondyra added a commit to mbondyra/kibana that referenced this pull request Jul 14, 2020
@mbondyra
[Lens] XY chart -long legend overflows chart in editor Feature:Lens (e…
aaf0895 
…lastic#70702)
mbondyra added a commit that referenced this pull request Jul 15, 2020
@mbondyra
[Lens] XY chart -long legend overflows chart in editor Feature:Lens (#…
9f8b365 
…70702) (#71752)
@mbondyra mbondyra deleted the lens/legend_stretch branch July 16, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees
No one assigned
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Lens] XY chart -long legend overflows chart in editor
6 participants
@mbondyra @elasticmachine @flash1293 @wylieconlon @kibanamachine @cchaos