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

Only show mouseover-title on text in navigationItem #1098

Merged
merged 1 commit into from Jun 16, 2020

Conversation

jotoeri
Copy link
Contributor

@jotoeri jotoeri commented May 17, 2020

On forms, @jancborchardt recognized, that the actions-menu of navigationItems shows the form-title on mouseover.
This PR now shifts the title-attribute, into the Title-span, so it is only visible there. Actions-Menu as well as counter do not show the title anymore.

Before (imagine a cursor above the small label):
grafik
grafik

After:
grafik

grafik

@jotoeri jotoeri added the 3. to review Waiting for reviews label May 17, 2020
@jotoeri jotoeri mentioned this pull request May 17, 2020
20 tasks
@jancborchardt

This comment has been minimized.

@jotoeri

This comment has been minimized.

@jancborchardt
Copy link
Contributor

The idea for the title on the navigation item is that it shows the entire title if it’s ellipsized – which I would also keep (of course in the example it doesn’t really get across as it’s a very short name).

The point in the issue was about the same title being shown on the actions, which seems like duplication. There my question for @skjnldsv if this was because of some accessibility guidelines. :)

@jotoeri

This comment has been minimized.

@jotoeri jotoeri changed the title Remove Mouse-over title on navigationItem Only show mouseover-title on icon and text in navigationItem May 18, 2020
@jotoeri jotoeri force-pushed the fix/navigation_mouseover_title branch from f6fcb25 to e5766a3 Compare May 22, 2020 17:08
@jotoeri
Copy link
Contributor Author

jotoeri commented May 22, 2020

Rebased & Squashed.

@jancborchardt
Copy link
Contributor

jancborchardt commented May 22, 2020

Ok, i see. So only showing it on title & icon? (see fixup)

Yup! I would even say that only showing it on the title is fine. :)
As the icon could be something else like status (in Forms) or color (in Calendar).

Signed-off-by: Jonas Rittershofer <jotoeri@users.noreply.github.com>
@jotoeri jotoeri force-pushed the fix/navigation_mouseover_title branch from e5766a3 to 1597302 Compare May 22, 2020 23:25
@jotoeri jotoeri changed the title Only show mouseover-title on icon and text in navigationItem Only show mouseover-title on text in navigationItem May 22, 2020
@jotoeri
Copy link
Contributor Author

jotoeri commented May 22, 2020

:D ok. So now it's only on title.^^

Copy link
Contributor

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

👍 design-wise! :)

@@ -130,7 +129,9 @@ Just set the `pinned` prop.
class="app-navigation-entry-icon">
<slot v-if="!loading" v-show="isIconShown" name="icon" />
</div>
<span class="app-navigation-entry__title">{{ title }}</span>
<span class="app-navigation-entry__title" :title="title">
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need a title here as there is some actual visible text content with the exact same information.

https://a11yproject.com/posts/title-attributes/

Short answer: Don’t use them, except in special circumstances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From side of accessibility, yes, you're right.
Jan's argument to not fully remove it, was for long titles, that get cut and only show the three dots. So they are not visible anymore, and then to be able to see the full text?

Here on forms:
grafik

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @juliushaertl, but as @jotoeri said the initial reason why we added the title is to offer a way for mouse-users to see the whole title when it’s longer than the width of the navigation.

So to just fix the issue at hand with it appearing too often but not to cut off the entire functionality, I’d say we should merge this. :) We could then separately look into if we can e.g. only show the tooltip if the text overflows in the first place, if possible?

Copy link
Contributor

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Fine by me then :)

@juliushaertl juliushaertl merged commit 2758635 into master Jun 16, 2020
@juliushaertl juliushaertl deleted the fix/navigation_mouseover_title branch June 16, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants