[Lens] Revisit flyout fields style from column to row compressed by dej611 · Pull Request #120103 · elastic/kibana

dej611 · 2021-12-01T14:18:57Z

Summary

Mostly changed field aspect from columnCompressed to rowCompressed with additional refactoring of EuiSpacers, and Top Values Rank direction component.
Updated test to match the new components.

Some questions

given the increased space for form fields, would it make sense to move some of the numeric fields - when possible - to range sliders?
- For instance Top values > Number of values has both min and max defined. But other fields with min/max defined (i.e. Percentile) can apply as well.

cc @MichaelMarcialis

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

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-12-01T16:13:17Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

MichaelMarcialis · 2021-12-01T16:55:14Z

given the increased space for form fields, would it make sense to move some of the numeric fields - when possible - to range sliders?

For instance Top values > Number of values has both min and max defined. But other fields with min/max defined (i.e. Percentile) can apply as well.

I'd be OK with adding a supporting range slider to the number inputs assuming 1) there is enough space (i.e. using row compressed), 2) the related number inputs do not support an infinite range of numbers (i.e. they have a finite min and max), and 3) the changes made via the slide provide the user direct feedback via representation in visualization. I just want to make sure we're not relying on sliders alone for items that require precision, as the sliders aren't particularly good at that.

dej611 · 2021-12-01T17:02:52Z

I'd be OK with adding a supporting range slider to the number inputs assuming 1) there is enough space (i.e. using row compressed), 2) the related number inputs do not support an infinite range of numbers (i.e. they have a finite min and max), and 3) the changes made via the slide provide the user direct feedback via representation in visualization. I just want to make sure we're not relying on sliders alone for items that require precision, as the sliders aren't particularly good at that.

Agree on all three conditions.

In both Top values > number of values and Percentile cases:

the field would show both a range slide and a number input for precision
min and max are a finite value.

Will update the PR with the new components.

mbondyra · 2021-12-02T10:34:39Z

This is not caused by your PR, but could you also fix mobile view for the flyout?

We could either overwrite the max-width style to 100%:

or adding to mobile media query .lnsDimensionContainer left: 10vw; to align it to the right.

dej611 · 2021-12-02T11:59:34Z

Fixed the mobile layout for the flyout as proposed by @mbondyra :

Replaced the Percentile numeric input with a range input:

While the idea of replacing the Top values > Number of values sounded nice, when implemented I've realized that:

it doesn't look that good for the best practices of having low values
it encourages to set higher values for the function

flash1293 · 2021-12-02T13:06:08Z

@dej611 With your additions for mobile does this fix #103778 as well?

dej611 · 2021-12-02T13:13:14Z

@dej611 With your additions for mobile does this fix #103778 as well?

That looks pretty close @mbondyra 's issue. I'll add the z-index thingy to sortout the search bar problem and link the issue in the main description of the PR 🎉

dej611 · 2021-12-02T14:10:15Z

Sorted also the z-index problem with the search bar:

dej611 · 2021-12-03T11:25:59Z

@elasticmachine merge upstream

mbondyra · 2021-12-03T13:52:33Z

Tested on FF and it looks as expected. Approved!

mbondyra · 2021-12-03T13:52:58Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/advanced_options.tsx

      {popoverOptions.length > 0 && (
        <EuiText textAlign="right">
-          <EuiSpacer size="s" />
+          {/* <EuiSpacer size="s" /> */}


nit: can it be removed?

…factor/flyout-ui

.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss

MichaelMarcialis

Great work on this, @dej611. As mentioned in my comment below, I made a quick design PR for the flyout issue on smaller viewport sizes. Once that PR is merged into this branch, that will address my only concerns. Otherwise, this looks good to go. Approving now, assuming you'll be able to merge that design PR in without issue.

.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss

* allow flyout to go full width on small viewports * remove hiding of layer panel when flyout open

mbondyra · 2021-12-06T10:05:27Z

@elasticmachine merge upstream

kibana-ci · 2021-12-13T13:08:41Z

💚 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.1MB	1.1MB	-2.1KB

History

💛 Build #11229 was flaky 6eda17a
💚 Build #11020 succeeded 78fb97b
💚 Build #10943 succeeded 2b37a0f
💚 Build #10917 succeeded 76f14f2
💚 Build #10659 succeeded 43c2111
💚 Build #10617 succeeded 90ff5c2

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

…stic#120103) * 💄 Revisit form style + spacers * ✅ Fix tests * ✨ Replace percentile input with range * 💄 Fix mobile layout * 💄 Make searchbar go below flyout * 🔥 Remove unused jsx * [Lens] Refactor Flyout Design Updates (#14) * allow flyout to go full width on small viewports * remove hiding of layer panel when flyout open Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Michael Marcialis <michael@marcial.is>

dej611 added 2 commits December 1, 2021 14:27

💄 Revisit form style + spacers

71cb7aa

✅ Fix tests

3f46a75

dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:skip This PR does not require backporting v8.1.0 labels Dec 1, 2021

dej611 marked this pull request as ready for review December 1, 2021 16:13

dej611 requested a review from a team as a code owner December 1, 2021 16:13

dej611 added 2 commits December 2, 2021 12:52

✨ Replace percentile input with range

c555d3e

💄 Fix mobile layout

90ff5c2

dej611 requested a review from a team as a code owner December 2, 2021 11:55

💄 Make searchbar go below flyout

43c2111

Merge branch 'main' into refactor/flyout-ui

76f14f2

mbondyra approved these changes Dec 3, 2021

View reviewed changes

mbondyra reviewed Dec 3, 2021

View reviewed changes

dej611 added 2 commits December 3, 2021 14:55

🔥 Remove unused jsx

d54fd8f

Merge branch 'refactor/flyout-ui' of github.com:dej611/kibana into re…

2b37a0f

…factor/flyout-ui

ryankeairns reviewed Dec 3, 2021

View reviewed changes

.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss Outdated Show resolved Hide resolved

MichaelMarcialis approved these changes Dec 3, 2021

View reviewed changes

.../plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss Outdated Show resolved Hide resolved

[Lens] Refactor Flyout Design Updates (#14)

78fb97b

* allow flyout to go full width on small viewports * remove hiding of layer panel when flyout open

kibanamachine and others added 2 commits December 6, 2021 05:05

Merge branch 'main' into refactor/flyout-ui

6eda17a

Merge remote-tracking branch 'upstream/main' into refactor/flyout-ui

a3ead7c

dej611 merged commit 7176dd5 into elastic:main Dec 14, 2021

Conversation

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

Uh oh!

Summary

Some questions

Checklist

Risk Matrix

For maintainers

Uh oh!

elasticmachine commented Dec 1, 2021

Uh oh!

MichaelMarcialis commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dej611 commented Dec 1, 2021

Uh oh!

mbondyra commented Dec 2, 2021

Uh oh!

dej611 commented Dec 2, 2021

Uh oh!

flash1293 commented Dec 2, 2021

Uh oh!

dej611 commented Dec 2, 2021

Uh oh!

dej611 commented Dec 2, 2021

Uh oh!

dej611 commented Dec 3, 2021

Uh oh!

mbondyra commented Dec 3, 2021

Uh oh!

mbondyra Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mbondyra commented Dec 6, 2021

Uh oh!

kibana-ci commented Dec 13, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

dej611 commented Dec 1, 2021 •

edited

Loading

MichaelMarcialis commented Dec 1, 2021 •

edited

Loading