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.

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

[Maps] Fix text readability on map scale, attribution, and coordinate controls #189639

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

jsanz
Copy link
Member

@jsanz jsanz commented Jul 31, 2024

Summary

Fixes #128783

This PR fixes the visualization of the texts in Maps scale bar, attribution, and coordinate controls with a dark background.

Context

While checking on the scale bar component of the map I saw this SCSS mixin that seemed to be broken.

Originally I thought on fixing the shadow but the readability of the text is not great for dark background (see 7f2a698)

Adding a shadow
before after
2024-07-31_16-17 2024-07-31_16-18

So I decided to propose to fix this by adding a background as is usual in other platforms.

image

image

image

@jsanz jsanz added release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Maps labels Jul 31, 2024
@jsanz
Copy link
Member Author

jsanz commented Jul 31, 2024

/ci

@jsanz jsanz requested a review from a team July 31, 2024 14:28
@jsanz
Copy link
Member Author

jsanz commented Jul 31, 2024

/ci

@jsanz jsanz changed the title [Maps] Fix text shadow on map controls [Maps] Fix text readability on map scale, attribution, and coordinate controls Jul 31, 2024
@jsanz jsanz marked this pull request as ready for review August 1, 2024 08:39
@jsanz jsanz requested a review from a team as a code owner August 1, 2024 08:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review and screenshot review only

@jsanz jsanz enabled auto-merge (squash) August 1, 2024 15:33
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.

Nice update, @jsanz! I agree with you that applying a background color is probably our best bet here. However, I do worry that the use of a semi-transparent background color could potentially result in a text color contrast that fails accessibility standards in some scenarios. As such, it may be better to use a non-transparent background color with a text color of sufficient contrast.

For example, I've quickly mocked up an example below in light and dark mode that uses euiTheme.colors.body for the background and euiTheme.colors.subduedText for the text. Since the background has no transparency, there should be no fear of contrast issues or issues across light/dark mode. For the purposes of my example, I also repositioned the text containers to the extreme edges of the map to prevent visual interference with inner portions of the map. Thoughts?

map-light

map-dark

@jsanz
Copy link
Member Author

jsanz commented Aug 20, 2024

Sorry for the delayed response, just getting back from PTO.

Appreciate the comments @MichaelMarcialis!! I've tried the text to be $euiTextSubduedColor but it was too low contrast so I'm sticking to $euiTextColor but switching the background for $euiPageBackgroundColor (is that the equivalent in Saas for the euiTheme.colors.body?) at 4630fd4.

At the beginning I was trying to find a background that would fit well with all dark/light/classic map styles (that can be selected independently of the Kibana theme). By the screenshots below, following the Kibana style as the other map widgets feels pretty aligned and the contrast is always good as well. What do you think @nreese @nickpeihl?

Screenshots

Light mode - auto basemap

image

Dark mode - auto basemap

image

Dark mode - classic

image

Dark mode - light

image

Dark mode - light with a tint

image

Light mode - dark

image

@jsanz jsanz disabled auto-merge August 20, 2024 14:01
@nickpeihl
Copy link
Member

At the beginning I was trying to find a background that would fit well with all dark/light/classic map styles (that can be selected independently of the Kibana theme). By the screenshots below, following the Kibana style as the other map widgets feels pretty aligned and the contrast is always good as well. What do you think @nreese @nickpeihl?

Improving contrast by using the Kibana style for the background lgtm!

@MichaelMarcialis
Copy link
Contributor

I've tried the text to be $euiTextSubduedColor but it was too low contrast so I'm sticking to $euiTextColor but switching the background for $euiPageBackgroundColor (is that the equivalent in Saas for the euiTheme.colors.body?) at 4630fd4.

@jsanz: Yeah, $euiPageBackgroundColor should be the Sass equivalent for euiTheme.colors.body. I also checked the color contrast for using $euiTextSubduedColor over that background color, and it's passing (4.81:1), so you should be good there.

@jsanz jsanz enabled auto-merge (squash) August 23, 2024 08:46
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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 3.0MB 3.0MB -122.0B

History

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

@jsanz jsanz disabled auto-merge August 26, 2024 08:34
@jsanz jsanz requested a review from MichaelMarcialis August 26, 2024 08:35
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.

Approving now to unblock. Would it be worth my opening a separate issue for some of the suggested tweaks in my earlier comment's mockups (for things like text color, positioning, etc.)?

@jsanz
Copy link
Member Author

jsanz commented Aug 27, 2024

Approving now to unblock. Would it be worth my opening a separate issue for some of the suggested tweaks in my earlier comment's mockups (for things like text color, positioning, etc.)?

yeah sorry, I failed to comment about the positioning 🤦 We would appreciate a separate issue!!

@jsanz jsanz merged commit 32dd373 into elastic:main Aug 27, 2024
23 checks passed
@jsanz jsanz deleted the ems/scale-control-background branch August 27, 2024 09:06
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 27, 2024
… controls (elastic#189639)

## Summary

Fixes elastic#128783

This PR fixes the visualization of the texts in Maps scale bar,
attribution, and coordinate controls with a dark background.

## Context

While checking on the scale bar component of the map I saw this SCSS
mixin that seemed to be broken.

Originally I thought on fixing the shadow but the readability of the
text is not great for dark background (see
7f2a698)

<details><summary>Adding a shadow</summary>

| before | after |
| :--:| :--:|

|![2024-07-31_16-17](https://github.com/user-attachments/assets/23239b0d-826e-482e-ac67-fe7faa50d696)
|
![2024-07-31_16-18](https://github.com/user-attachments/assets/dffe0873-86dd-46b3-877e-ff4f5ef92c32)
|
</details>

So I decided to propose to fix this by adding a background as is usual
in other platforms.

![image](https://github.com/user-attachments/assets/7a7d0d11-1cf6-4961-b872-a5247c930363)

![image](https://github.com/user-attachments/assets/2c3d893b-f380-4724-9ad5-93adb6ec9fb6)

![image](https://github.com/user-attachments/assets/1a9becbc-40b1-4631-8360-f2a1bfafc9c2)

(cherry picked from commit 32dd373)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.15

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Aug 27, 2024
…rdinate controls (#189639) (#191408)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Maps] Fix text readability on map scale, attribution, and coordinate
controls (#189639)](#189639)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jorge
Sanz","email":"jorge.sanz@elastic.co"},"sourceCommit":{"committedDate":"2024-08-27T09:06:15Z","message":"[Maps]
Fix text readability on map scale, attribution, and coordinate controls
(#189639)\n\n## Summary\r\n\r\nFixes #128783\r\n\r\nThis PR fixes the
visualization of the texts in Maps scale bar,\r\nattribution, and
coordinate controls with a dark background.\r\n\r\n##
Context\r\n\r\nWhile checking on the scale bar component of the map I
saw this SCSS\r\nmixin that seemed to be broken.\r\n\r\nOriginally I
thought on fixing the shadow but the readability of the\r\ntext is not
great for dark background
(see\r\n7f2a698f4401c9b22f5c526ad8cfaa7839e30282)\r\n\r\n<details><summary>Adding
a shadow</summary>\r\n\r\n| before | after |\r\n| :--:| :--:|
\r\n\r\n|![2024-07-31_16-17](https://github.com/user-attachments/assets/23239b0d-826e-482e-ac67-fe7faa50d696)\r\n|\r\n![2024-07-31_16-18](https://github.com/user-attachments/assets/dffe0873-86dd-46b3-877e-ff4f5ef92c32)\r\n|\r\n</details>\r\n\r\nSo
I decided to propose to fix this by adding a background as is
usual\r\nin other
platforms.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/7a7d0d11-1cf6-4961-b872-a5247c930363)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/2c3d893b-f380-4724-9ad5-93adb6ec9fb6)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/1a9becbc-40b1-4631-8360-f2a1bfafc9c2)","sha":"32dd3730fa70acdb2e37918861c32537ffd2fc7e","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","backport:prev-minor","Feature:Maps","v8.16.0"],"title":"[Maps]
Fix text readability on map scale, attribution, and coordinate
controls","number":189639,"url":"https://github.com/elastic/kibana/pull/189639","mergeCommit":{"message":"[Maps]
Fix text readability on map scale, attribution, and coordinate controls
(#189639)\n\n## Summary\r\n\r\nFixes #128783\r\n\r\nThis PR fixes the
visualization of the texts in Maps scale bar,\r\nattribution, and
coordinate controls with a dark background.\r\n\r\n##
Context\r\n\r\nWhile checking on the scale bar component of the map I
saw this SCSS\r\nmixin that seemed to be broken.\r\n\r\nOriginally I
thought on fixing the shadow but the readability of the\r\ntext is not
great for dark background
(see\r\n7f2a698f4401c9b22f5c526ad8cfaa7839e30282)\r\n\r\n<details><summary>Adding
a shadow</summary>\r\n\r\n| before | after |\r\n| :--:| :--:|
\r\n\r\n|![2024-07-31_16-17](https://github.com/user-attachments/assets/23239b0d-826e-482e-ac67-fe7faa50d696)\r\n|\r\n![2024-07-31_16-18](https://github.com/user-attachments/assets/dffe0873-86dd-46b3-877e-ff4f5ef92c32)\r\n|\r\n</details>\r\n\r\nSo
I decided to propose to fix this by adding a background as is
usual\r\nin other
platforms.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/7a7d0d11-1cf6-4961-b872-a5247c930363)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/2c3d893b-f380-4724-9ad5-93adb6ec9fb6)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/1a9becbc-40b1-4631-8360-f2a1bfafc9c2)","sha":"32dd3730fa70acdb2e37918861c32537ffd2fc7e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/189639","number":189639,"mergeCommit":{"message":"[Maps]
Fix text readability on map scale, attribution, and coordinate controls
(#189639)\n\n## Summary\r\n\r\nFixes #128783\r\n\r\nThis PR fixes the
visualization of the texts in Maps scale bar,\r\nattribution, and
coordinate controls with a dark background.\r\n\r\n##
Context\r\n\r\nWhile checking on the scale bar component of the map I
saw this SCSS\r\nmixin that seemed to be broken.\r\n\r\nOriginally I
thought on fixing the shadow but the readability of the\r\ntext is not
great for dark background
(see\r\n7f2a698f4401c9b22f5c526ad8cfaa7839e30282)\r\n\r\n<details><summary>Adding
a shadow</summary>\r\n\r\n| before | after |\r\n| :--:| :--:|
\r\n\r\n|![2024-07-31_16-17](https://github.com/user-attachments/assets/23239b0d-826e-482e-ac67-fe7faa50d696)\r\n|\r\n![2024-07-31_16-18](https://github.com/user-attachments/assets/dffe0873-86dd-46b3-877e-ff4f5ef92c32)\r\n|\r\n</details>\r\n\r\nSo
I decided to propose to fix this by adding a background as is
usual\r\nin other
platforms.\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/7a7d0d11-1cf6-4961-b872-a5247c930363)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/2c3d893b-f380-4724-9ad5-93adb6ec9fb6)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/1a9becbc-40b1-4631-8360-f2a1bfafc9c2)","sha":"32dd3730fa70acdb2e37918861c32537ffd2fc7e"}}]}]
BACKPORT-->

Co-authored-by: Jorge Sanz <jorge.sanz@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Maps release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.1 v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User configurable scale color for Kibana maps
7 participants