[Lens] Fix value popover spacing by dej611 · Pull Request #103081 · elastic/kibana

dej611 · 2021-06-23T12:37:11Z

Summary

Before:

After:

Previous version

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-06-23T12:37:13Z

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

dej611 · 2021-06-23T14:32:50Z

@elasticmachine merge upstream

wylieconlon

This solution appears to work, but does not seem like it will scale for all the other inputs we could add in the future. What if we used a CSS sibling selector to do the spacing instead, like the & + & or & > * + * { pattern that is used in a few places?

dej611 · 2021-06-24T12:48:08Z

Reimplemented via CSS:

MichaelMarcialis

Left one comment/question regarding the margin styling.

MichaelMarcialis · 2021-06-24T16:46:27Z

x-pack/plugins/lens/public/xy_visualization/xy_config_panel.scss

+  .lnsXyToolbar__visualOption + .lnsXyToolbar__visualOption {
+    margin-top: $euiSizeL;
+  }


Is this style addition necessary? As far as I'm aware, adjacent sibling EuiFormRow components are already styled by EUI to have a margin-top applied (as shown in the screenshot below). If indeed not needed, I suggest removing this style and related class additions.

For some reason some Spacers have been iteratively added to the popup, in addition to the regular EuiFormRow spacing, until it reached the initial broken state.
This PR was meant to restore a state where Spacers were uniformly distributed.

Looking at other popups in the same UI it appears that they have only the EuiFormRow margin, which you pointed at. It makes more sense to keep only the original EUI spacing, so I'll remove the additional one from the PR.

wylieconlon

Haven't retested but thanks for doing the CSS solution, hopefully this will be easier to maintain now.

dej611 · 2021-06-29T15:06:20Z

@elasticmachine merge upstream

dej611 · 2021-06-29T15:09:20Z

Removed any custom spacing and CSS and made EUI do its thing, now popover spacing is consistent within the UI:

MichaelMarcialis

Thanks for making those changes, @dej611! LGTM.

kibanamachine · 2021-06-29T17:09:29Z

💚 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	-315.0B

History

💚 Build #134050 succeeded ed162ef
💔 Build #134020 failed 85b0561
💚 Build #133578 succeeded b450d78
💔 Build #133527 failed 51a8413

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-30T08:11:36Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (178 commits) [test] Migrating to kbn_archiver from es_archiver - for the Maps app (elastic#103028) [Reporting] Reintroduce "ILM policy for managing reporting indices" (elastic#103850) [Security Solution][Endpoint] Allow activity log scrolling on small screens (elastic#103852) Allow zero (0) to unset unenroll_timeout field (elastic#103790) [TSVB] Metric count is depicted as `-` instead of 0 (elastic#103717) [Query] Es query/field base (elastic#103177) Remove add data button from nav (elastic#103810) Fix telemetry advanced setting style (elastic#103838) [Transform] Fix default naming and sorting fields suggestion for `top_metrics` agg (elastic#103690) [APM] use conventional error rate color for correlations (elastic#103500) Endpoint Telemetry: Agents Metrics + Policy Config / Response (elastic#102171) [Alerting] Fixed search results are not updated when search term is removed on Rules and Connectors page (elastic#103663) fix too many rernders (elastic#103672) [APM] Add “Analyze Data” button (elastic#103485) [Lens] Fix value popover spacing (elastic#103081) [TSVB] Fix TSVB is not reporting all categories of Elasticsearch error (elastic#102926) [SECURITY] Adds security links to doc link service (elastic#102676) Update dependency @elastic/charts to v31 (elastic#102078) [Security Solution][CTI] Investigation time enrichment UI (elastic#103383) Adds ECS guide to doc links service (elastic#102246) ... # Conflicts: # x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx

💄 Fix spacing in popover

51a8413

dej611 requested a review from a team June 23, 2021 12:37

Merge branch 'master' into fix/value_popover

b450d78

wylieconlon approved these changes Jun 23, 2021

View reviewed changes

💄 Reimplementing via css

85b0561

dej611 requested a review from a team as a code owner June 24, 2021 12:47

🔥 Remove unused imports

ed162ef

MichaelMarcialis reviewed Jun 24, 2021

View reviewed changes

wylieconlon reviewed Jun 24, 2021

View reviewed changes

💄 Removed extra class

c4ec1ff

Merge branch 'master' into fix/value_popover

7ff15d3

dej611 requested a review from MichaelMarcialis June 29, 2021 15:09

MichaelMarcialis approved these changes Jun 29, 2021

View reviewed changes

dej611 merged commit 21858a5 into elastic:master Jun 30, 2021

dej611 deleted the fix/value_popover branch June 30, 2021 08:06

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 30, 2021

[Lens] Fix value popover spacing (elastic#103081)

c254399

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine mentioned this pull request Jun 30, 2021

[7.x] [Lens] Fix value popover spacing (#103081) #103842

Merged

kibanamachine added a commit that referenced this pull request Jun 30, 2021

[Lens] Fix value popover spacing (#103081) (#103842)

002427c

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

spalger added the v7.15.0 label Jul 7, 2021

Conversation

dej611 commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 23, 2021

Uh oh!

dej611 commented Jun 23, 2021

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 commented Jun 24, 2021

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelMarcialis Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

dej611 Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 commented Jun 29, 2021

Uh oh!

dej611 commented Jun 29, 2021

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 29, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Jun 30, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dej611 commented Jun 23, 2021 •

edited

Loading