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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 23, 2024
Merged

fix(NcHeaderMenu): mouse in now pointer #5101

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions src/components/NcHeaderMenu/NcHeaderMenu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,21 @@ export default {
class="header-menu"
v-on="listeners">
<!-- Trigger -->
<button :id="isNav ? triggerId : null"
<NcButton :id="isNav ? triggerId : null"
ref="trigger"
class="header-menu__trigger button-vue"
type="tertiary-no-background"
class="header-menu__trigger"
:aria-label="ariaLabel"
:aria-describedby="description ? descriptionId : null"
:aria-controls="`header-menu-${id}`"
:aria-expanded="opened.toString()"
@click.prevent="toggleMenu">
<!-- @slot Icon trigger slot. Make sure the svg path
<template #icon>
<!-- @slot Icon trigger slot. Make sure the svg path
is at least 16px. Usually mdi icon works at 20px -->
<slot name="trigger" />
</button>
<slot name="trigger" />
</template>
</NcButton>

<span v-if="description"
:id="descriptionId"
Expand Down Expand Up @@ -115,9 +118,15 @@ import GenRandomId from '../../utils/GenRandomId.js'
import { clickOutsideOptions } from '../../mixins/index.js'
import { getTrapStack } from '../../utils/focusTrap.js'

import NcButton from '../NcButton/index.js'

export default {
name: 'NcHeaderMenu',

components: {
NcButton,
},

directives: {
ClickOutside,
},
Expand Down Expand Up @@ -339,17 +348,9 @@ $externalMargin: 8px;
height: var(--header-height);

&__trigger {
display: flex;
align-items: center;
justify-content: center;
width: var(--header-height);
height: var(--header-height);
margin: 0;
padding: 0;
cursor: pointer;
opacity: .85;
background-color: transparent;
border: none;

// header is filled with primary or image background
filter: none !important;
Expand All @@ -363,8 +364,9 @@ $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 •
edited
Loading

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)

outline: none;
#{&}__trigger:focus-visible {
outline: none !important;
box-shadow: none !important;
}

&__wrapper {
Expand Down
Loading