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(dropdown,listbox,listbox-multi): fixing issues with clicking options and focus ring with mouse #11

Merged
merged 10 commits into from
Oct 24, 2023

Conversation

jordanstith15
Copy link
Collaborator

@jordanstith15 jordanstith15 commented Oct 13, 2023

Closes COMUI-2342 and COMUI-2359

@github-actions
Copy link

Demo will be published at https://apps.inindca.com/common-ui-docs/genesys-webcomponents/feature/COMUI-2342

@jordanstith15 jordanstith15 changed the title fix(dropdown,listbox,listbox-multi): fixing issues with clicking options and focus ring with mouse [WIP] fix(dropdown,listbox,listbox-multi): fixing issues with clicking options and focus ring with mouse Oct 13, 2023
@jordanstith15
Copy link
Collaborator Author

Having issues figuring out why using keyboard navigation adds an extra pixel on the four corners of the listbox only in Chrome. Any ideas would be appreciated

@gavin-everett-genesys
Copy link
Collaborator

Having issues figuring out why using keyboard navigation adds an extra pixel on the four corners of the listbox only in Chrome. Any ideas would be appreciated

Can you post a screenshot of what you are seeing

@jordanstith15
Copy link
Collaborator Author

jordanstith15 commented Oct 13, 2023

Having issues figuring out why using keyboard navigation adds an extra pixel on the four corners of the listbox only in Chrome. Any ideas would be appreciated

Can you post a screenshot of what you are seeing

A screenshot is here in the figma, second column under gux-announce-beta. It's really hard to see since it's only like 1 pixel

@jordanstith15 jordanstith15 changed the title [WIP] fix(dropdown,listbox,listbox-multi): fixing issues with clicking options and focus ring with mouse fix(dropdown,listbox,listbox-multi): fixing issues with clicking options and focus ring with mouse Oct 13, 2023
Comment on lines +151 to 153
&:focus-within:has(:focus-visible) {
@include focus.gux-focus-ring;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You wont be able to use the :has selector unfortunately as its not supported by firefox. There probably is some sort of workaround though :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I know. this is a case though that I don't think it's a super big deal unless it breaks the css entirely.. iirc all this one does is show the focus while you're typing after hitting enter during keyboard navigation or clicking on a filterable dropdown, which might be weird anyways

@gavin-everett-genesys
Copy link
Collaborator

Is it possible to to make the gux-option fill the entire space of the gux-listbox so that there is not white space to the left and right of the gux-option ?

@jordanstith15
Copy link
Collaborator Author

Is it possible to to make the gux-option fill the entire space of the gux-listbox so that there is not white space to the left and right of the gux-option ?

that is a token issue with the padding. UX already knows and I think has a fix ready for whenever they can set up tokens on the new repo

@gavin-everett-genesys
Copy link
Collaborator

In the dropdown-multi when the tag appears with the number of options selected. I think there should be a gap of 8px applied between the number and the close icon. I think this can be applied to the .gux-tag class. I dont think a token exists for the gap yet though according to a figma comment.

@jordanstith15
Copy link
Collaborator Author

jordanstith15 commented Oct 18, 2023

In the dropdown-multi when the tag appears with the number of options selected. I think there should be a gap of 8px applied between the number and the close icon. I think this can be applied to the .gux-tag class. I dont think a token exists for the gap yet though according to a figma comment.

There is a token for gap (--gse-ui-tag-removable-gap), but it's set to 4px. Just using the flexbox gap wouldn't work due to that gap being only in specific situations (that is the only situation now but wanted to leave open for future), so I set it as margin-left on the remove icon.
The figma comment is for the weird gap they were expecting after the remove icon. I think it looks weird that way and there isn't a token for it either. I don't remember the designs looking that way at first too

<slot></slot>
</div>
) as JSX.Element;
return (<slot slot="popup"></slot>) as JSX.Element;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this one line of code in its own function or can we declare it inline from where this function is called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to keep it as a separate function, especially for any possible future changes. But no strong reasons otherwise

@@ -28,6 +28,7 @@
outline: var(--gse-ui-menu-option-focus-border-width)
var(--gse-ui-menu-option-focus-border-style)
var(--gse-ui-menu-option-focus-border-color);
outline-offset: -2px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add a token for this or document it needs a token?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm unsure if we should. This is kind of going against the design because the exact design doesn't work correctly. I think we will be discussing this with UX at some point

@jordanstith15 jordanstith15 merged commit f077205 into beta/v4 Oct 24, 2023
2 checks passed
@daragh-king-genesys daragh-king-genesys deleted the feature/COMUI-2342 branch November 10, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants