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] Fix help popovers #103314

Merged
merged 5 commits into from
Jun 30, 2021
Merged

[Lens] Fix help popovers #103314

merged 5 commits into from
Jun 30, 2021

Conversation

flash1293
Copy link
Contributor

Part of #97421

New look:
Screenshot 2021-06-24 at 17 58 27

@flash1293 flash1293 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.14.0 labels Jun 24, 2021
@flash1293 flash1293 mentioned this pull request Jun 24, 2021
Closed
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 marked this pull request as ready for review June 28, 2021 07:32
@flash1293 flash1293 requested a review from a team June 28, 2021 07:32
@flash1293 flash1293 requested a review from a team as a code owner June 28, 2021 07:32
@elasticmachine
Copy link
Contributor

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

wylieconlon
wylieconlon approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

LGTM

MichaelMarcialis
MichaelMarcialis approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Left comments regarding a possibly cleaner way of dealing with the popover padding without using negative margining, but will leave the ultimate decision up to you. Approving now so I don't hold you up.

x-pack/plugins/lens/public/indexpattern_datasource/help_popover.tsx
@@ -65,7 +65,6 @@ export const HelpPopover = ({
isOpen={isOpen}
ownFocus
panelClassName="lnsHelpPopover__panel"
panelPaddingSize="none"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than removing the panelPaddingSize prop here and relying on negative margining to get the popover contents flush with the popover edges, it might be cleaner to just leave this panelPaddingSize="none" in place and instead put a paddingSize="m" prop on the EuiPopoverTitle component below. Not super important, but just thought it might be cleaner overall.

x-pack/plugins/lens/public/indexpattern_datasource/help_popover.scss Outdated
@@ -6,6 +6,7 @@
@include euiYScrollWithShadows;
max-height: 40vh;
padding: $euiSizeM;
margin: -$euiSizeM;
Copy link
Contributor

Choose a reason for hiding this comment

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

As per my previous comment, you can do away with the negative margining CSS if you go the route of applying a paddingSize prop to the EuiPopoverTitle component.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 1.5MB 1.5MB +13.0B

History

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

@flash1293
Copy link
Contributor Author

THanks @MichaelMarcialis , much better

@flash1293 flash1293 merged commit e04f995 into elastic:master Jun 30, 2021
@flash1293 flash1293 mentioned this pull request Jun 30, 2021
Merged
flash1293 added a commit to flash1293/kibana that referenced this pull request Jun 30, 2021
@flash1293
[Lens] Fix help popovers (elastic#103314)
4f203a5
flash1293 added a commit that referenced this pull request Jun 30, 2021
@flash1293
[Lens] Fix help popovers (#103314) (#103903)
21e87b7
flash1293 added a commit to flash1293/kibana that referenced this pull request Jul 7, 2021
@flash1293
[Lens] Fix help popovers (elastic#103314)
31a44ff
@flash1293 flash1293 mentioned this pull request Jul 7, 2021
Merged
@spalger spalger added the v7.15.0 label Jul 7, 2021
flash1293 added a commit that referenced this pull request Jul 9, 2021
@flash1293
[Lens] Fix help popovers (#103314) (#104639)
3e7e8f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelMarcialis MichaelMarcialis MichaelMarcialis approved these changes

@wylieconlon wylieconlon wylieconlon 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.14.0 v7.15.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flash1293 @elasticmachine @kibanamachine @wylieconlon @MichaelMarcialis @spalger