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

Jump to bottom

docs: add stories for UiMenu #489

Merged
merged 35 commits into from
Jul 31, 2024
Merged

docs: add stories for UiMenu #489

merged 35 commits into from
Jul 31, 2024

Conversation

kubaszajna
Copy link
Contributor

@kubaszajna kubaszajna commented May 16, 2024 •
edited
Loading

Description

Implement ListBox for MultiSelect lists

Related Issue

https://infermedica.atlassian.net/browse/MGPFE-57

Motivation and Context

We want to achieve the same level of accessibility across the MGP modules

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@kubaszajna kubaszajna self-assigned this May 16, 2024
@kubaszajna kubaszajna requested a review from a team as a code owner May 16, 2024 06:35
@pspaczek
Copy link
Contributor

feat: implement ListBox for MultiSelect lists docs: add stories ... for UiList

@kubaszajna kubaszajna marked this pull request as draft May 16, 2024 12:13
@kubaszajna kubaszajna changed the title feat: implement ListBox for MultiSelect lists docs: add stories for UiList May 28, 2024
@kubaszajna kubaszajna marked this pull request as ready for review May 28, 2024 22:28
AlanLes
AlanLes previously approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@AlanLes AlanLes left a comment

Choose a reason for hiding this comment

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

lgtm 👍

pspaczek
pspaczek suggested changes May 29, 2024
View reviewed changes
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
sonya0504
sonya0504 requested changes May 29, 2024
View reviewed changes
Copy link
Contributor

@sonya0504 sonya0504 left a comment

Choose a reason for hiding this comment

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

Review done, please check my comments

src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
src/components/organisms/UiList/stories/ListBox.vue Outdated
@@ -0,0 +1,74 @@
<template>
<UiList
Copy link
Contributor

Choose a reason for hiding this comment

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

Keyboard interaction doesn't work as expected. For example: moving by keyboard from one option to another should be available by key arrow up and arrow down, now is possible using Tab and Tab+shift.

Please check the sections Keyboard interaction and Aria and add changes to code

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check this section in confluence and fix keyboard navigation:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I left a few more links with details on how to use ul role="listbox and an alternative way with select:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/listbox_role
https://www.w3.org/WAI/ARIA/apg/patterns/listbox/examples/listbox-rearrangeable/
I should help to implement the best solution 😍

src/components/organisms/UiList/stories/ListBox.vue Outdated Show resolved Hide resolved
@sonya0504
Copy link
Contributor

I can still see the bugs/problems which I reported earlier. Could you recheck my comments and fix them?

@kubaszajna kubaszajna marked this pull request as draft June 6, 2024 10:07
@kubaszajna kubaszajna marked this pull request as ready for review June 12, 2024 07:24
@sonya0504
Copy link
Contributor

sonya0504 commented Jun 12, 2024 •
edited
Loading

[EDIT: FIXED by @kubaszajna ]

focus and hover look different than it is in the design:

Focus:
Story:
image
The Design has the whole row focused:
image

Hover:
Story:
image
Design:
image

@kubaszajna kubaszajna requested review from pspaczek and sonya0504 July 30, 2024 13:23
@pspaczek pspaczek self-requested a review July 30, 2024 13:40
pspaczek
pspaczek previously approved these changes Jul 30, 2024
View reviewed changes
Copy link
Contributor

@pspaczek pspaczek left a comment

Choose a reason for hiding this comment

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

@kubaszajna @pspaczek
fix: update hasbutton key handler
dbacf40 
Co-authored-by: Przemek Spaczek <[email protected]>
@kubaszajna kubaszajna requested a review from pspaczek July 30, 2024 13:42
pspaczek
pspaczek previously approved these changes Jul 30, 2024
View reviewed changes
sonya0504
sonya0504 reviewed Jul 31, 2024
View reviewed changes
Copy link
Contributor

@sonya0504 sonya0504 left a comment

Choose a reason for hiding this comment

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

image

Comparing design new story is narrower and has shadow. Could change it like it is in design?

src/components/organisms/UiMenu/stories/HasButton.vue Outdated Show resolved Hide resolved
src/components/organisms/UiMenu/stories/HasButton.vue Show resolved Hide resolved
@sonya0504
Copy link
Contributor

Review done, I left 3 small comments

@kubaszajna kubaszajna requested review from sonya0504 and pspaczek July 31, 2024 09:54
sonya0504
sonya0504 previously approved these changes Jul 31, 2024
View reviewed changes
pspaczek
pspaczek approved these changes Jul 31, 2024
View reviewed changes
src/components/organisms/UiMenu/stories/HasButton.vue Show resolved Hide resolved
src/components/organisms/UiMenu/stories/HasButton.vue Outdated
--popover-content-padding-block: 0;
--popover-content-padding-inline: 0;

box-shadow: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.figma.com/design/2p8nMM7W88FU6r0kA52fAA/MGP-Component-Library?node-id=4184-91237&t=bDdIbs8g2z7SuojX-4

please ask the design why they change the master component in this place. Master has a shadow. IMO it shouldn't be changed in this PR

Suggested change
box-shadow: none;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I’m waiting for response in case I will correct it later.

@pspaczek pspaczek merged commit 79f2787 into infermedica:develop Jul 31, 2024
1 check passed
@pspaczek pspaczek mentioned this pull request Jul 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanLes AlanLes AlanLes left review comments

@pspaczek pspaczek pspaczek approved these changes

@sonya0504 sonya0504 sonya0504 approved these changes

@jbucior jbucior Awaiting requested review from jbucior

@kubajmarek kubajmarek Awaiting requested review from kubajmarek

Assignees

@kubaszajna kubaszajna

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kubaszajna @pspaczek @sonya0504 @AlanLes