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

Unify tooltip and headline and add the corresponding icon #1455

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

jschleic
Copy link
Contributor

@jschleic jschleic commented Dec 8, 2023

Tooltips for all the buttons now match the headline on top of the dialogs (with #1449 the headings should probably also be filled from the new constants)

Added h3 heading in dialogs without any headline and re-arranged some h3 / h4 inconsistencies.

This PR separates out the first part of #1454 as separate PR.

* search and share icons on the left get an `title` attribute
* all panels start with an `h3` headline including the corresponding icon
* thus add smaller versions of the 24px-icons to 16.svg or 16-white.svg
* unify strings for button and headline to match
* left align share icon
* add light tilelayer icon in "view" mode
@@ -325,6 +325,7 @@ L.DomUtil.createFieldset = (container, legend, options) => {
L.DomUtil.createButton = (className, container, content, callback, context) => {
const el = L.DomUtil.add('button', className, container, content)
el.type = 'button'
el.title = content
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of adding the same title as the content for a button element?

@jschleic
Copy link
Contributor Author

jschleic commented Dec 12, 2023 via email

@davidbgk
Copy link
Contributor

Do you see any negative side effect for other text buttons?

If the content of the tooltip is the same as the displayed label of the button, I think it might confuse the user with extra visual information that is not really pertinent. In other places we tried to put additional informations in that case (like keyboard shortcuts or additional context) and there might still be some inconsistencies(?).

This is a thin line between helping the user and not overloading the interface 🤸

@jschleic
Copy link
Contributor Author

jschleic commented Dec 14, 2023

If the content of the tooltip is the same as the displayed label of the button, I think it might confuse the user with extra visual information that is not really pertinent. In other places we tried to put additional informations in that case (like keyboard shortcuts or additional context) and there might still be some inconsistencies(?).

If that is the desired strategy, there are a lot of inconsistencies:

Most of the Close buttons already have same title and text:

const closeLink = L.DomUtil.create('li', 'umap-close-link', actionsContainer)
L.DomUtil.add('i', 'umap-close-icon', closeLink)
const label = L.DomUtil.create('span', '', closeLink)
label.title = label.textContent = L._('Close')

const label = L.DomUtil.create('span', '', closeButton)
label.title = label.textContent = L._('Close')

const label = L.DomUtil.create('span', '', closeButton)
label.title = label.textContent = L._('Close')

All labels in edit forms seem to apply the strategy to set the same text and title:

this.label.textContent = this.label.title = this.options.label

So for me the only inconsistency seemed to be the missing title for share and search buttons on the left, which I want to fix in this PR:
grafik

Would you be happy with a specific title for those buttons on the left? Then I'll replace

el.title = content

with individually setting for those buttons:

    shareButton.title = L._('Share, download and embed this map')

@davidbgk
Copy link
Contributor

If that is the desired strategy, there are a lot of inconsistencies

Fair enough 😅

I would say that is not blocking for that PR, let's do that as a future step toward cohesion 👍

@davidbgk davidbgk merged commit b25bb16 into umap-project:master Dec 14, 2023
12 checks passed
@jschleic jschleic deleted the tooltip-and-headline branch December 15, 2023 09:10
@jschleic
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants