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

fix(NcHeaderMenu): mouse in now pointer #5101

Conversation

emoral435
Copy link
Contributor

@emoral435 emoral435 commented Jan 20, 2024

☑️ For nextcloud/server#42959

🖼️ Screenshots

🏡 After

firefox_UBN04LrlJO

🚧 Tasks

  • make sure the the pointer is cursor on all elements that can be input into the #icon slot

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@emoral435 emoral435 added bug Something isn't working 3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels Jan 20, 2024
@emoral435 emoral435 added this to the 8.5.0 milestone Jan 20, 2024
@emoral435 emoral435 self-assigned this Jan 20, 2024
@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch from 3c20dc6 to 9e60e4f Compare January 22, 2024 13:41
@emoral435 emoral435 requested a review from ShGKme January 22, 2024 13:44
@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch from 9e60e4f to 8e457e3 Compare January 22, 2024 19:39
@emoral435 emoral435 requested a review from susnux January 22, 2024 19:39
@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch from 8e457e3 to e3bf888 Compare January 22, 2024 20:03
Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Trigger button still needs adjustments to be styled the same as before

Before After
image image image

@@ -363,10 +364,6 @@ $externalMargin: 8px;
opacity: 1;
}

&__trigger:focus-visible {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to stay, see @Pytal comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried leaving this in to take out the border outline, but since the NcButton has the !important styling precedent on its outline and bordering styling, I had to use :deep styling in the newest PR.

NcButton
image

And this did not work :(
image

Copy link
Contributor

Choose a reason for hiding this comment

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

yes because both have the same specificity, you also can just increase it, currently the style is just on .header-menu__trigger, but it should work with .header-menu .header-menu__trigger:focus-visible.

Something like:

.header-menu {
    // ...
    #{&}__trigger:focus-visible {
        // here
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if it is better just an option ;)

Copy link
Contributor Author

@emoral435 emoral435 Jan 23, 2024

Choose a reason for hiding this comment

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

Ahhh, only now do I get what you mean! I changed it to how you requested, actually, because your solution uses something similar with the Unified Search fix in nextcloud/server#42978 , I will use your solution (plus I try not to use :deep as much as possible)

@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch from e3bf888 to 38f8ed9 Compare January 23, 2024 00:24
@emoral435 emoral435 requested review from Pytal and susnux January 23, 2024 00:30
Copy link
Contributor

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Code looks code, style looks style!

@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch 2 times, most recently from 1aefe9b to 9b77ed5 Compare January 23, 2024 02:22
@emoral435 emoral435 force-pushed the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch from 9b77ed5 to 6cdf138 Compare January 23, 2024 02:26
@emoral435 emoral435 enabled auto-merge January 23, 2024 02:27
@emoral435 emoral435 merged commit a5134f5 into master Jan 23, 2024
15 checks passed
@emoral435 emoral435 deleted the fix/a11y/42959/no-mouse-pointer-at-search-and-notifications-icons branch January 23, 2024 02:37
@Pytal
Copy link
Contributor

Pytal commented Jan 24, 2024

There is a slight regression where the trigger button width of 50px is being overidden by NcButton width of 44px @emoral435

image

@emoral435
Copy link
Contributor Author

Gotcha 👍 will fix that right now, and send out the new version. Embarrassed that I did not notice this 🫠 thank you!

@emoral435
Copy link
Contributor Author

Regression fixed in #5129 ! @Pytal

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 accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants