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

Improve navigation sub-menu and tabs effects #2971

Merged
merged 11 commits into from
Jul 23, 2020

Conversation

kimsible
Copy link
Contributor

@kimsible kimsible commented Jul 15, 2020

Needs #2977 to update new icons.

This PR proposes to improve the navigation links effects :

  • Prefer changing text color instead of text bold when switching between active and non-active link to avoid to resize effects ; => use opacity: 0.6 on non-active links

  • Add transparent borders on non-active links to avoid involuntary resize between links or tabs ;

  • Change color on hover event for tabs ;

  • Border-bottom black on hover event for links (sub-menu) ; => only opacity: 1

  • Remove margin-top on active link (sub-menu) ;

  • Sub-menu fixed ;
    usefull on touchdevices

  • Improve Dropdown : active class on active link + remove hover open + close outside click only
    make sure the sub-menu is not too long horizontally - less confusing nav and more common

  • Remove labels Dropdown items and put them in H1 with icons after the sub-menu
    cleaner, simplier ...

1-videos
1-subscriptions
1-reports
1-playlists
1-imports
1-history
1-channels

@rigelk rigelk self-assigned this Jul 15, 2020
@rigelk rigelk added the UI non-trivial UI changes, that might need discussion label Jul 15, 2020
@rigelk
Copy link
Collaborator

rigelk commented Jul 15, 2020

Prefer changing text color instead of text bold when switching between active and non-active link to avoid to resize effects

Rather than color change (I don't like the switch to orange*), I would rather use something like box-shadow: 1px 0px 0px black for focused options. It doesn't render so well on Safari apparently though, so I'd use something like https://stackoverflow.com/a/32570813 albeit with a data-content attribute instead of title. WDYT?

Border-bottom black on hover event for links (sub-menu)

Why?

Remove margin-top on active link (sub-menu)

I can't see this in the diff.


*to argue a bit more, it is not just that I don't like the change, but more that bold should be used with caution. Bolding caries meaning and directs the user sight towards the boldened part. Blurring the line by just changing the color is not a fitting equivalent.

@kimsible
Copy link
Contributor Author

Remove margin-top on active link (sub-menu)

I can't see this in the diff.

This is that commit : 9eac1ac

@kimsible
Copy link
Contributor Author

kimsible commented Jul 15, 2020

@rigelk

Rather than color change (I don't like the switch to orange*), I would rather use something like box-shadow: 1px 0px 0px black for focused options. It doesn't render so well on Safari apparently though, so I'd use something like https://stackoverflow.com/a/32570813 albeit with a data-content attribute instead of title. WDYT?

I've tryed, but it seems lots of efforts on each nav.

I have another solution which can visually be similar to bold on active :

  • Use a lighter bold than semi-bold
  • Revert to Foreground / black color active link
  • Use lighten($color, $amount) Sass for non-active links opacity: 0.6

Border-bottom black on hover event for links (sub-menu)

Why?

Also removes the border-bottom on hover, and use only an opacity: 1 on the text

Screenshot_2020-07-15 Users list - mon instance

@kimsible kimsible marked this pull request as ready for review July 17, 2020 22:41
@kimsible kimsible changed the title Improve navigation links effects Improve navigation sub-menu and tabs effects Jul 17, 2020
@kimsible
Copy link
Contributor Author

@rigelk the new changes have been discussed with the same graphic designer added in the credits of this PR : #2977.

I updated the description to explain some choices, especially for the Dropdown-menu.

Now it looks very cleaner, what do you think ?

Needs the new icons in #2977.

@kimsible
Copy link
Contributor Author

@rigelk What do you think about the new improvements ? Displaying the sub-item label as a title with its icon considerably reduce the width size of the sub-menu, so it corrects all our problems of horizontal scroll with responsive considering we do not have more than 4 items in the user sub-menu and 6 in the admin sub-menu.

@Chocobozzz Chocobozzz merged commit ed5bb51 into Chocobozzz:develop Jul 23, 2020
@Chocobozzz
Copy link
Owner

Thanks @rigelk & @kimsible

@@ -4,26 +4,28 @@
<a *ngIf="menuEntry.routerLink" [routerLink]="menuEntry.routerLink" routerLinkActive="active" class="title-page title-page-settings">{{ menuEntry.label }}</a>

<div *ngIf="!menuEntry.routerLink" ngbDropdown class="parent-entry"
#dropdown="ngbDropdown" (mouseleave)="closeDropdownIfHovered(dropdown)">
#dropdown="ngbDropdown" autoClose="outside">
Copy link
Owner

Choose a reason for hiding this comment

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

When we click on an element, the dropdown is not automatically closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I explained it also in the description :

Improve Dropdown : active class on active link + remove hover open + close outside click only
make sure the sub-menu is not too long horizontally - less confusing nav and more common

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it is for 2 things :

  • Accessibility, make sure the dropdown is still openened after click
  • Avoid epileptic effect with the active class onclick (2 events trigged at the same time)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kimsible for the accessibility, how is this part of WCAG 2.1? for the epilepitc nature, it does not flash per WCAG 2.3.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Invoking W3C spec would not help us here...but only crushing propositions with an authority argument.
I don't see what you mean and I'm really opened to make any changes, maybe removing the active class and back to the autoClose=true (inside and outsite).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Invoking W3C spec would not help us here

I'm just trying to understand what hurdles we have to care about when it comes to designing the dropdown. If you just say "Accessibility", then I look up what corresponds to it. If you explain the reasoning behind classifying your proposition as more accessible, then we can compare.

Usually dropdowns are kept open for some elements (i.e. nested dropdowns), not just for navigation.

Copy link
Owner

Choose a reason for hiding this comment

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

Accessibility, make sure the dropdown is still openened after click

I don't think users expect a menu to be still opened after a click. Every time we want to go to another page, we have to click 2 times.

Maybe a menu/menubar role could resolve the accessibility issue of dropdown auto close.

Copy link
Contributor Author

@kimsible kimsible Jul 29, 2020

Choose a reason for hiding this comment

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

I don't think users expect a menu to be still opened after a click. Every time we want to go to another page, we have to click 2 times.

This PR goes back to the old effect #3023 but another PR should be created for the accessibility issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI non-trivial UI changes, that might need discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants