[RA-3888] feat(menu): adds menu component based on mui menu #558

santanasara · 2024-04-25T13:12:30Z

adds new menu component based on the material ui menu

Description

Adiciona componente Menu baseado no componente do Material UI
https://jirasoftware.catho.com.br/browse/RA-3888

Setup

to view the components behavior, run yarn storybook

Review guide

Coverage (coverage status should not regress)
- yarn test:coverage
Unit tests (yarn test:components)
Regression
- first start the storybook for regression tests(and keep it open): yarn test:regression:storybook;
- then run the regression tests: yarn test:regression
Code review
TypeScript updated

A11y review

A11y checks
- accessible via keyboard?
- Identifiable through assistive technology (screen-reader chrome extension)?
- Semantically correct html elements (or if necessary identified by WAI-ARIA)?
- Is there a deficiency in color contrast?

Browsers review

adds new menu component based on the material ui menu

MarcosViniciusPC

[] Code review
[x] unit
[x] regression
[] Doc

MarcosViniciusPC · 2024-04-29T17:33:06Z

components/Menu/Menu.jsx

+      id="menu"
+      data-testid="menu"


If the objective is to do tests, I believe that these props can be removed. Use screen.getByRole('presentation') in the test to get the menu.

Suggested change

id="menu"

data-testid="menu"

MarcosViniciusPC · 2024-04-29T17:41:05Z

components/Menu/Menu.jsx

+      id="menu"
+      data-testid="menu"
+      theme={materialThemeOverride}
+      aria-labelledby="button"


It seems like a problem because whoever uses the component doesn't know that it needs to have an element with id="button" for the menu to be accessible, you know? Furthermore, the component is already somewhat accessible. Below is the component structure

MarcosViniciusPC · 2024-04-29T17:43:25Z

components/Menu/Menu.jsx

+        const onClickFunc = () => {
+          const { handleClick } = item;
+          handleClose();
+          handleClick();
+        };


This function here causes it to be recalculated every time the list is rendered, which may result in performance losses. What do you think about moving it after the props are destructured?

MarcosViniciusPC · 2024-04-29T17:47:12Z

components/Menu/Menu.jsx

+  /** If true, the component is shown. */
+  open: PropTypes.bool,
+  /** Menu contents, has a content parameter, an id and a handleClick function. */
+  items: PropTypes.array,


What do you think about defining exactly what data format should be in this array and thus avoiding unwanted surprises? For example, if someone passes an object without the "content" prop, our component will break, as we use "content" in the MenuItem.

It is based on the same data format defined in the typescript file here in the component

MarcosViniciusPC · 2024-04-29T17:52:02Z

components/Menu/Menu.unit.test.jsx

+      anchorEl: mockAnchorEl,
+      handleClose: mockHandleClose,
+    });
+    const menu = queryByTestId('menu');


If you followed the suggestion above, getByRole('presentation') should work here and in other cases where getByTestId is used

MarcosViniciusPC · 2024-04-29T17:57:56Z

stories/Menu/Menu.stories.jsx

+    <div>
+      <Button
+        id="button"
+        aria-controls={open ? 'menu' : undefined}


Suggested change

aria-controls={open ? 'menu' : undefined}

aria-controls="presentation"

MarcosViniciusPC · 2024-04-29T17:58:10Z

stories/Menu/Menu.stories.jsx

+        id="button"
+        aria-controls={open ? 'menu' : undefined}
+        aria-haspopup="true"
+        aria-expanded={open ? 'true' : undefined}


Suggested change

aria-expanded={open ? 'true' : undefined}

aria-expanded={open}

MarcosViniciusPC · 2024-04-29T17:59:13Z

stories/Menu/Menu.stories.jsx

+        aria-hidden="true"
+        aria-label="Menu"
+        role="button"
+        id="button"
+        aria-controls={open ? 'menu' : undefined}
+        aria-haspopup="true"
+        aria-expanded={open ? 'true' : undefined}
+        onClick={handleClick}


Similar situation to that mentioned previously

MarcosViniciusPC · 2024-04-29T17:59:59Z

components/Menu/index.d.ts

+    horizontal: 'left' | 'center' | 'right';
+  };
+  keepMounted?: boolean;
+  handleClose?: (event: {}, reason: 'backdropClick' | 'escapeKeyDown') => void;


Suggested change

handleClose?: (event: {}, reason: 'backdropClick' | 'escapeKeyDown') => void;

onClose?: (event: {}, reason: 'backdropClick' | 'escapeKeyDown') => void;

onClose seems to make more sense for those who use the component

MarcosViniciusPC · 2024-04-29T18:00:09Z

components/Menu/index.d.ts

+export interface MenuItemProps {
+  id: string | number;
+  content: React.ReactNode;
+  handleClick: Function;


Suggested change

handleClick: Function;

onClick: Function;

onClick seems to make more sense for those who use the component

I was thinking here and I believe that this prop doesn't even make much sense to stay in the items object. What about including it as a prop (onClick: (item) => {}) of the component instead of placing it inside the object?

I think your suggestion adds a bit more complexity and my implementation with object literals is superior in this case making the code neater for the user to implement

improves types and component readbility by screen readers

improves test by using role instead of id

MarcosViniciusPC

[] Code
[] Unit
[] Regression
[] Doc

MarcosViniciusPC · 2024-04-30T19:07:57Z

stories/Menu/Menu.stories.jsx

+    <div>
+      <button
+        type="button"
+        aria-labelledby="Menu"


When aria-label is defined, aria-labelledby is not necessary as they have the same purpose

Suggested change

aria-labelledby="Menu"

MarcosViniciusPC · 2024-04-30T19:08:35Z

stories/Menu/Menu.stories.jsx

+      <Button
+        aria-controls="presentation"
+        aria-haspopup="true"
+        aria-expanded={open}


Suggested change

aria-expanded={open}

aria-expanded={open}

aria-label="Menu"

components/Menu/Menu.jsx

Co-authored-by: MarcosViniciusPC <[email protected]>

improves aria labels on documentation component

alizeleal

I can't navigate the menu options using tab
Is this the expected behavior?

alizeleal · 2024-05-02T18:39:59Z

Also, he regression tests don't contemplate the opened menu options, only the always visible button

santanasara · 2024-05-02T18:47:35Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

alizeleal · 2024-05-02T19:06:54Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

In the reference, I cna navigate with the arrows up and down, but not in this component.

santanasara · 2024-05-02T20:42:16Z

Also, he regression tests don't contemplate the opened menu options, only the always visible button

I tried running the regressions on the open menu but I kept getting this error when running yarn test:regression. Do you have a suggestion on how to make it work? I'm not familiar with this "loki" library

Failed with "Unable to get position of selector "#regression-test > *". Review the `chromeSelector` option and 
       make sure your story doesn't crash." after 4 tries

navigation issues

santanasara · 2024-05-02T20:57:59Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

In the reference, I cna navigate with the arrows up and down, but not in this component.

I installed the component on talent-selection (talent-selection_app) and noticed some changes @MarcosViniciusPC suggested broke the arrow navigation and also broke other behaviours (like the opening behaviour). So I reverted some changes back to what they were before and now arrow navigation seems to work again (see video evidence).
Gravação de tela de 02-05-2024 17:35:59.webm

sonarqubecloud · 2024-05-02T20:58:23Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
16.3% Duplication on New Code

See analysis details on SonarCloud

MarcosViniciusPC · 2024-05-03T17:52:43Z

new menu component based on the material

Due to the urgency, we will create a technical debt for us here at ACME to improve these tests in the future.

MarcosViniciusPC · 2024-05-03T17:58:48Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

In the reference, I cna navigate with the arrows up and down, but not in this component.

I installed the component on talent-selection (talent-selection_app) and noticed some changes @MarcosViniciusPC suggested broke the arrow navigation and also broke other behaviours (like the opening behaviour). So I reverted some changes back to what they were before and now arrow navigation seems to work again (see video evidence). Gravação de tela de 02-05-2024 17:35:59.webm

How did you do the tests? I tried to simulate the navigation behavior using the arrow keys here and was unable to navigate between items. @alizeleal , confirm to me later if it worked for you. If not, let me know so I can create a task to correct the navigation.

MarcosViniciusPC

[x] Code review
[x] Unit
[] Regression

santanasara · 2024-05-03T18:07:37Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

In the reference, I cna navigate with the arrows up and down, but not in this component.

I installed the component on talent-selection (talent-selection_app) and noticed some changes @MarcosViniciusPC suggested broke the arrow navigation and also broke other behaviours (like the opening behaviour). So I reverted some changes back to what they were before and now arrow navigation seems to work again (see video evidence). Gravação de tela de 02-05-2024 17:35:59.webm

How did you do the tests? I tried to simulate the navigation behavior using the arrow keys here and was unable to navigate between items. @alizeleal , confirm to me later if it worked for you. If not, let me know so I can create a task to correct the navigation.

I generated a build locally and installed it in the talent-selection application and tested there.

alizeleal · 2024-05-03T18:21:59Z

I can't navigate the menu options using tab Is this the expected behavior?

Yes, the material ui menu also doesn't navigate through tab https://mui.com/material-ui/react-menu/#basic-menu

In the reference, I cna navigate with the arrows up and down, but not in this component.

I installed the component on talent-selection (talent-selection_app) and noticed some changes @MarcosViniciusPC suggested broke the arrow navigation and also broke other behaviours (like the opening behaviour). So I reverted some changes back to what they were before and now arrow navigation seems to work again (see video evidence). Gravação de tela de 02-05-2024 17:35:59.webm

How did you do the tests? I tried to simulate the navigation behavior using the arrow keys here and was unable to navigate between items. @alizeleal , confirm to me later if it worked for you. If not, let me know so I can create a task to correct the navigation.

I generated a build locally and installed it in the talent-selection application and tested there.

It didn't work in the documentation

alizeleal

Approved with the technical debt for the keyboard navigation and regressive test

github-actions · 2024-05-03T18:58:41Z

🎉 This PR is included in version 9.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2024-05-16T19:27:21Z

🎉 This PR is included in version 10.0.0-beta.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(menu): adds menu component based on mui menu

3e5bb7f

adds new menu component based on the material ui menu

santanasara added the status:ready to review label Apr 25, 2024

santanasara self-assigned this Apr 25, 2024

MarcosViniciusPC requested review from MarcosViniciusPC and alizeleal April 25, 2024 18:55

MarcosViniciusPC added status:reviewing Someone is reviewing this PR and removed status:ready to review labels Apr 29, 2024

MarcosViniciusPC requested changes Apr 29, 2024

View reviewed changes

MarcosViniciusPC added status:fix status:suggestion and removed status:reviewing Someone is reviewing this PR labels Apr 29, 2024

santanasara added 2 commits April 30, 2024 09:44

refactor(menu): improves types and component readbility

d001939

improves types and component readbility by screen readers

test(menu): improves test by removing unecessary id

5d33faf

improves test by using role instead of id

alizeleal added status:reviewing Someone is reviewing this PR and removed status:fix labels Apr 30, 2024

MarcosViniciusPC requested changes Apr 30, 2024

View reviewed changes

santanasara and others added 2 commits April 30, 2024 19:31

fix(menu): content typing in menu items

507af29

Co-authored-by: MarcosViniciusPC <[email protected]>

refactor(menu): improves aria labels on documentation component

38e065d

improves aria labels on documentation component

alizeleal requested a review from MarcosViniciusPC May 2, 2024 18:28

alizeleal suggested changes May 2, 2024

View reviewed changes

alizeleal added question Further information is requested status:fix and removed status:reviewing Someone is reviewing this PR labels May 2, 2024

fix(menu): navigation issues

cb27ba9

navigation issues

MarcosViniciusPC approved these changes May 3, 2024

View reviewed changes

MarcosViniciusPC added status:pending review and removed status:fix labels May 3, 2024

alizeleal self-requested a review May 3, 2024 18:32

alizeleal approved these changes May 3, 2024

View reviewed changes

MarcosViniciusPC merged commit fc066e9 into catho:master May 3, 2024
3 checks passed

MarcosViniciusPC added status:approved and removed status:pending review question Further information is requested status:suggestion labels May 3, 2024

github-actions bot added the released label May 3, 2024

github-actions bot added the released on @beta label May 16, 2024

	aria-controls={open ? 'menu' : undefined}
	aria-controls="presentation"

	aria-expanded={open ? 'true' : undefined}
	aria-expanded={open}

	handleClose?: (event: {}, reason: 'backdropClick' \| 'escapeKeyDown') => void;
	onClose?: (event: {}, reason: 'backdropClick' \| 'escapeKeyDown') => void;

[RA-3888] feat(menu): adds menu component based on mui menu #558

[RA-3888] feat(menu): adds menu component based on mui menu #558

Conversation

santanasara commented Apr 25, 2024

Description

Setup

Review guide

A11y review

Browsers review

MarcosViniciusPC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosViniciusPC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alizeleal left a comment

Choose a reason for hiding this comment

alizeleal commented May 2, 2024

santanasara commented May 2, 2024

alizeleal commented May 2, 2024

santanasara commented May 2, 2024

santanasara commented May 2, 2024 • edited Loading

sonarqubecloud bot commented May 2, 2024

Quality Gate passed

MarcosViniciusPC commented May 3, 2024

MarcosViniciusPC commented May 3, 2024

MarcosViniciusPC left a comment

Choose a reason for hiding this comment

santanasara commented May 3, 2024

alizeleal commented May 3, 2024

alizeleal left a comment

Choose a reason for hiding this comment

github-actions bot commented May 3, 2024

github-actions bot commented May 16, 2024

santanasara commented May 2, 2024 •

edited

Loading