[K8] [Maps] Fix toolbar overlay styles #96352

elizabetdev · 2021-04-06T20:14:54Z

Summary

This PR is fixing:

No shadows in EuiButtonIcon in v8. This was fixed by wrapping the EuiButtonIcon in a EuiPanel.
Now we have better hover effects.
The top button Zoom In and Zoom Out are using mapbox buttons. The styles are now matching other buttons styles.
It was introduced styles to handle button groups. This could be used to add the new draw buttons that @aaronjcaldwell is working on. Basically to add a group of buttons we just need to:

<EuiPanel paddingSize="none" className="mapToolbarOverlay__buttonGroup">
  <EuiButtonIcon size="s"  iconType="node" />
  <EuiButtonIcon size="s"  iconType="mapMarker" />
</EuiPanel>

Checklist

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)
This was checked for cross-browser compatibility

elasticmachine · 2021-04-06T20:18:41Z

Pinging @elastic/kibana-gis (Team:Geo)

thomasneirynck

styles looks good, just one question wrt highlighting of the zoom +- and fit buttons.

is the highlighting effect supposed to be "sticky"

e.g. the hover-effect remains applied for a little bit, causing sometimes multiple buttons to be highlighted at the same time or remain active "after clicking". e.g. this is not the case for the wrench or go-to button.

kmartastic · 2021-04-06T23:00:04Z

e.g. the hover-effect remains applied for a little bit, causing sometimes multiple buttons to be highlighted at the same time or remain active "after clicking". e.g. this is not the case for the wrench or go-to button.

Good catch.

elizabetdev · 2021-04-07T11:00:17Z

e.g. the hover-effect remains applied for a little bit, causing sometimes multiple buttons to be highlighted at the same time or remain active "after clicking". e.g. this is not the case for the wrench or go-to button.

The active "after clicking" is the :focus state. It means the button is focused. For the go-to and wrench it doesn't happen because both have a popover. So once you click the button and the popover opens the focus goes to the popover.

But I decided to remove the focus state from the buttons because it can induce to think that the button is "active". When using the keyboard the focus ring appears and for now, I think it's a good compromise.

We also need to think about the active states for when @aaronjcaldwell introduces the draw tools. They need to show that this "tool" is active. So I think when we reach that point we can revisit the focus and active states. 👍🏽

kindsun · 2021-04-07T15:40:30Z

@elasticmachine merge upstream

kindsun · 2021-04-07T16:25:45Z

Overall looking really good! I see the Fit data to bounds button doesn't darken on hover. Is this intentional?

…ar-overlay

MichaelMarcialis

Hey, @miukimiu! Left two small comments for your review.

MichaelMarcialis · 2021-04-08T18:10:24Z

x-pack/plugins/maps/public/_mapbox_hacks.scss

+      @include kbnThemeStyle($theme: 'v7') {
+        &:first-child {
+          border-radius: $euiBorderRadius $euiBorderRadius 0 0;
+        }
+
+        &:last-child {
+          border-radius: 0 0 $euiBorderRadius $euiBorderRadius;
+        }
+      }
+
+      @include kbnThemeStyle($theme: 'v8') {
+        &:first-child {
+          border-radius: ($euiBorderRadius * (2 / 3)) ($euiBorderRadius * (2 / 3)) 0 0;
+        }
+
+        &:last-child {
+          border-radius: 0 0 ($euiBorderRadius * (2 / 3)) ($euiBorderRadius * (2 / 3));
+        }
+      }


Rather than styling the border-radius of these child buttons, could you instead remove these styles and just place an overflow: hidden; on the parent .mapboxgl-ctrl-group element? I believe that would achieve the same appearance with less SCSS. Thoughts?

Thanks @MichaelMarcialis! Great suggestion. I already added. 👍🏽

x-pack/plugins/maps/public/connected_components/toolbar_overlay/_toolbar_overlay.scss

…ar-overlay

kibanamachine · 2021-04-12T18:05:25Z

💚 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
`maps`	2.6MB	2.6MB	+3.7KB

History

💚 Build #118230 succeeded ad0e289
💔 Build #117910 failed 2451403
💚 Build #117800 succeeded 4280f1f
💚 Build #117795 succeeded 8dabf00
💔 Build #117785 failed 4533e73

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

MichaelMarcialis

LGTM!

* Fix toolbar overlay styles * More styles * Updating test * Better focus state for mapbox buttons * Mapbox buttons focus * Focus againa * Focus states again * no background only for focus not hover * Adding mixin for button group border radius Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

elizabetdev added 3 commits April 6, 2021 14:23

Fix toolbar overlay styles

b76c8f1

More styles

8d1ab56

Updating test

d3a28d1

elizabetdev requested a review from a team as a code owner April 6, 2021 20:14

elizabetdev added release_note:skip Skip the PR/issue when compiling release notes [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.13.0 labels Apr 6, 2021

elizabetdev requested a review from thomasneirynck April 6, 2021 20:19

thomasneirynck approved these changes Apr 6, 2021

View reviewed changes

elizabetdev added 4 commits April 7, 2021 10:58

Better focus state for mapbox buttons

4533e73

Mapbox buttons focus

2d9a426

Focus againa

8dabf00

Focus states again

4280f1f

Merge branch 'master' into k8/maps-fix-toolbar-overlay

2451403

elizabetdev added 2 commits April 8, 2021 14:34

no background only for focus not hover

0089940

Merge remote-tracking branch 'upstream/master' into k8/maps-fix-toolb…

ad0e289

…ar-overlay

MichaelMarcialis reviewed Apr 8, 2021

View reviewed changes

elizabetdev added 2 commits April 12, 2021 10:10

Merge remote-tracking branch 'upstream/master' into k8/maps-fix-toolb…

3d55464

…ar-overlay

Adding mixin for button group border radius

10b80a3

elizabetdev requested a review from MichaelMarcialis April 12, 2021 16:06

MichaelMarcialis approved these changes Apr 13, 2021

View reviewed changes

elizabetdev merged commit ba091c0 into elastic:master Apr 13, 2021

elizabetdev mentioned this pull request Apr 13, 2021

[7.x] [K8] [Maps] Fix toolbar overlay styles (#96352) #96991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[K8] [Maps] Fix toolbar overlay styles #96352

[K8] [Maps] Fix toolbar overlay styles #96352

elizabetdev commented Apr 6, 2021

elasticmachine commented Apr 6, 2021

thomasneirynck left a comment •

edited

Loading

kmartastic commented Apr 6, 2021

elizabetdev commented Apr 7, 2021

kindsun commented Apr 7, 2021

kindsun commented Apr 7, 2021

MichaelMarcialis left a comment •

edited

Loading

MichaelMarcialis Apr 8, 2021

elizabetdev Apr 12, 2021

kibanamachine commented Apr 12, 2021

MichaelMarcialis left a comment

[K8] [Maps] Fix toolbar overlay styles #96352

[K8] [Maps] Fix toolbar overlay styles #96352

Conversation

elizabetdev commented Apr 6, 2021

Summary

Checklist

elasticmachine commented Apr 6, 2021

thomasneirynck left a comment • edited Loading

Choose a reason for hiding this comment

kmartastic commented Apr 6, 2021

elizabetdev commented Apr 7, 2021

kindsun commented Apr 7, 2021

kindsun commented Apr 7, 2021

MichaelMarcialis left a comment • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis Apr 8, 2021

Choose a reason for hiding this comment

elizabetdev Apr 12, 2021

Choose a reason for hiding this comment

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

MichaelMarcialis left a comment

Choose a reason for hiding this comment

thomasneirynck left a comment •

edited

Loading

MichaelMarcialis left a comment •

edited

Loading