[Lens] fix amsterdam theme issues by mbondyra · Pull Request #97237 · elastic/kibana

mbondyra · 2021-04-15T12:04:59Z

Summary

Fixes #95054

adds border to the toolbar buttons (so far they are only used in Lens, and all of these borders make sense:

weird ranges/filter shadows fixed

suggestion shadows fixed

'add new layer' button to fill

adjusting border radius on workspace

adjust styles for filter by to look like ranges/filters aggregations filters

changing the inner focus style for field items

elasticmachine · 2021-04-15T12:29:03Z

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

flash1293

Add layer button should use color="text" in addition to fill, otherwise works great.
I don't think another review cycle is necessary, apporving

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx

wylieconlon

This is definitely an improvement, but I noticed a few inconsistencies that seem related to the new theme. Not sure if there's a simple fix:

Some popovers get a focus outline that is above the arrow, some popovers get a focus outline that is below the arrow, and some get no focus outline at all:
a. Correct behavior (below the arrow) is shown only in the chart switcher
b. Outline is below the arrow on all the toolbar buttons:

c. Outline is not shown on the first layer's index pattern switcher, but it is shown on the second layer

With two layers there is no padding underneath the Add Layer button:

mbondyra · 2021-04-19T07:44:10Z

Thanks for the feedback @wylieconlon! For now we won't address all the issues with outline and focus as we're waiting for the changes from eui. I've added your comments to the issue and fixed the missing pading for the add Layer button.

mbondyra · 2021-04-19T07:55:29Z

@elasticmachine merge upstream

ryankeairns · 2021-04-19T13:31:27Z

Side note: the Presentation team is using the same toolbar button styles, so I've opened an EUI issue to discuss whether these should ultimately move to EUI, in the future: elastic/eui#4730

ryankeairns

SCSS changes look good. Thanks for cleaning up the streets of Amsterdam 🧹

ryankeairns · 2021-04-20T01:24:34Z

src/plugins/kibana_react/public/toolbar_button/toolbar_button.scss

@@ -4,6 +4,10 @@

  // Some toolbar buttons are just icons, but EuiButton comes with margin and min-width that need to be removed


Not critical, but a comment pointing to elastic/eui#4730 would help us remember to replace/remove this code later.

kibanamachine · 2021-04-20T09:43:47Z

💚 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`	943.8KB	944.5KB	+712.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kibanaReact`	117.5KB	117.6KB	+72.0B

History

💚 Build #120618 succeeded d8bffbd
💚 Build #120566 succeeded 6d4326c
💚 Build #120444 succeeded 464c935
💚 Build #119981 succeeded cd01ece
💔 Build #119952 failed 4f2a86e

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

mbondyra · 2021-04-20T09:49:01Z

to be backported when 7.13 is detached

MichaelMarcialis · 2021-04-20T13:46:11Z

Side note: the Presentation team is using the same toolbar button styles, so I've opened an EUI issue to discuss whether these should ultimately move to EUI, in the future: elastic/eui#4730

@ryankeairns: Just FYI, assuming time permits, I may experiment with some additional toolbar treatments (background color, borders, shadows) to see if there is one clear direction that jives with Amsterdam best. Stay tuned.

ryankeairns · 2021-04-20T13:52:57Z

Side note: the Presentation team is using the same toolbar button styles, so I've opened an EUI issue to discuss whether these should ultimately move to EUI, in the future: elastic/eui#4730

@ryankeairns: Just FYI, assuming time permits, I may experiment with some additional toolbar treatments (background color, borders, shadows) to see if there is one clear direction that jives with Amsterdam best. Stay tuned.

Great! If you arrive at something you're satisfied with, routing it through that EUI issue would be much appreciated. Thanks

kibanamachine · 2021-04-22T09:49:39Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 97237 or prevent reminders by adding the backport:skip label.

* adding border to the toolbar buttons * Weird ranges/filters shadows/borders fixed * suggestion shadows fixed * adjusting border radius on workspace * add new layer button * adjust styles for filter by just like for filter agg * don't show outer style for selecting the field item * fix color button * add padding to the button * v8 conditional * fix v7 * Update toolbar_button.scss Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* adding border to the toolbar buttons * Weird ranges/filters shadows/borders fixed * suggestion shadows fixed * adjusting border radius on workspace * add new layer button * adjust styles for filter by just like for filter agg * don't show outer style for selecting the field item * fix color button * add padding to the button * v8 conditional * fix v7 * Update toolbar_button.scss Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

mbondyra added 7 commits April 15, 2021 14:03

adding border to the toolbar buttons

213d8c4

Weird ranges/filters shadows/borders fixed

f794908

suggestion shadows fixed

7600ed9

adjusting border radius on workspace

d7c4d6f

add new layer button

1a4c2e8

adjust styles for filter by just like for filter agg

23ff329

don't show outer style for selecting the field item

4f2a86e

mbondyra added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens labels Apr 15, 2021

mbondyra marked this pull request as ready for review April 15, 2021 12:29

mbondyra requested a review from a team April 15, 2021 12:29

mbondyra requested a review from a team as a code owner April 15, 2021 12:29

flash1293 approved these changes Apr 15, 2021

View reviewed changes

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.tsx Show resolved Hide resolved

fix color button

cd01ece

mbondyra force-pushed the lens/fix_amsterdam_theme branch from 009b156 to cd01ece Compare April 15, 2021 13:37

elastic deleted a comment from kibanamachine Apr 15, 2021

wylieconlon approved these changes Apr 15, 2021

View reviewed changes

add padding to the button

f8d6b3b

Merge branch 'master' into lens/fix_amsterdam_theme

464c935

This was referenced Apr 19, 2021

Shared toolbar - match button styles to Lens #97454

Closed

Add toolbar button styles elastic/eui#4730

Closed

mbondyra added 2 commits April 19, 2021 15:47

v8 conditional

6d4326c

fix v7

d8bffbd

ryankeairns approved these changes Apr 20, 2021

View reviewed changes

Update toolbar_button.scss

997cb01

mbondyra merged commit b227b8f into elastic:master Apr 20, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 22, 2021

mbondyra mentioned this pull request Apr 22, 2021

[7.x] [Lens] fix amsterdam theme issues (#97237) #97974

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 22, 2021

		@@ -4,6 +4,10 @@

		// Some toolbar buttons are just icons, but EuiButton comes with margin and min-width that need to be removed

Conversation

mbondyra commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Apr 15, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

mbondyra commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbondyra commented Apr 19, 2021

Uh oh!

ryankeairns commented Apr 19, 2021

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

ryankeairns Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 20, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

Uh oh!

mbondyra commented Apr 20, 2021

Uh oh!

MichaelMarcialis commented Apr 20, 2021

Uh oh!

ryankeairns commented Apr 20, 2021

Uh oh!

kibanamachine commented Apr 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mbondyra commented Apr 15, 2021 •

edited

Loading

mbondyra commented Apr 19, 2021 •

edited

Loading