feat(fluentUI-migration): ActiontButton fluentui v8 to v9 migration #7401

v-rakeshsh · 2024-07-30T06:45:25Z

Details

Action Button migration from V8 to V9

V8 Component	V9 Component	Reference Link
Action Button	Button	https://react.fluentui.dev/?path=/docs/components-button-button--docs
Primary Button	Button	https://react.fluentui.dev/?path=/docs/components-button-button--docs
CommandBarButton	Combination of Menu, MenuTrigger, MenuButton, MenuPopover, MenuList	1. MenuButton - https://react.fluentui.dev/?path=/docs/components-button-menubutton--docs 2. Menu - https://react.fluentui.dev/?path=/docs/components-menu-menu--docs 3. MenuList - https://react.fluentui.dev/?path=/docs/components-menu-menulist--docs
IconProps for Action Button and action button internally takes care of showing the icon	Created a component src/common/icons/fluentui-v9-icons.tsx to consume @fluentui/react-icons icons	Cell

Styling Improvements:

Updated the width property from 100% to auto for better layout flexibility in multiple snapshot files.
Added new CSS classes for better button and icon styling, including hover and focus states.
Adjusted font-weight to normal for better text consistency.

Component Enhancements:

Added FluentUIV9Icon import and updated the components to use the new icon.
Improved the components with new styles and updated its component structure to use Fluent UI v9 components.

Motivation

Migrate Action Button of V8 to V9 Button in all locations of code.
Changed types to align with V9 button.
Changed styling to align with old UI.
Fixed test cases to cover >90% coverage.

Context

UI locations for all the files are mentioned with images in below file:

ActionButtonComparison

Due to FluentUI V9 Migration there are slight differences between old and new Icons. PFA screenshots below.

More Actions, Collapse all Icons

Expand, Move to Assessment Icons

Export result, Save Assessment, Load Assessment, Start over Icons

File issue, Copy failure details Icons

Note :

isNarrowMode property is added in details-view-command-bar.tsx and other related files to differentiate the rendering of button in normal mode and menu in minimize mode.
SaveAssessmentDialog is now moved out of SaveAssessmentButton file as with v9 button migration to menu, dialog in narrow mode was not rendering as it was getting close once menu button is clicked.
Accessibility test has a failure for aria-hidden-focus. There is an existing issue with fluentui and as per them these are Tabster dummy inputs and they must be added to the exception list. They are intentionally aria - hidden as their purpose is to redirect focus to the correct element right when they receive focus and not having aria - hidden will result in the screen readers choking starting to announce them.
Please refer for more information on the error:[Bug]: Tablist - ARIA hidden element must not be focusable or contain focusable elements fluentui#25133
Changed buttonRef: IRefObject; to ButtonRefFunction, as ButtonRefFunction can be used in future when we migrate the dialog to v9 from V8.
Changed /load-assessment-button.tsx to functional component from class component so that i can consume makeStyles hooks, as hooks cannot be used in class components, hence converted to Functional component.
Removed IPoint because its deprecated in latest Fluent UI V8.
Removed expand-collapse-all-button.scss because to consume makeStyles hooks and to use V9 themes, hence created expand-collapse-all-button.-styles.tsx.

Technical Debt:
These will be take care in up coming PR for v8 to v9 migration.
IButton:

We are stilling using IButton from v8 for ref object in src\DetailsView\components\details-view-command-bar.tsx, because when we tried to use useRef instead of IButton, we have converted the component from class to functional component to consume useRef hook.
But after converting to functional component and using useRef, focus back to kebab button is not going in narrow mode when we close the dialog.

Card footer button scroll behaviour

If multiple issues are shown in the card footer, when we expand all issues, and if we click on the first issue section button, scroll goes to the bottom of the page i.e, the focus goes to the last section.

Closing dialog focus issue

In Narromode,When we open the dialog on click of export or save assessment or start over, and when we close the dialog, focus is not going back to the Kebab button, as dialog is still in v8, we will take care of this focus back issues during v9 dialog migration.

Focus style inconsistency

We explicitly added focus styling for buttons to show focus like kebab button / buttons, after closing the file issue dialog, but when we use keyboard to navigate to these kebab or other buttons, browser default focus styles are getting applied, for which it might look thick border for keyboard focus, but on close of dialog, the focus goes to button with little thin border.

Pull request checklist

Addresses an existing issue: #0000
Ran yarn fastpass
Added/updated relevant unit test(s) (and ran yarn test)
Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
(UI changes only) Added screenshots/GIFs to description above
(UI changes only) Verified usability with NVDA/JAWS

…osoft/accessibility-insights-web into v-rakesh/actionbutton-migration

…es, format & lint fixes

… CG Issue

[initState

madalynrose

This is a huge effort, well done! I have some questions about things I was confused about and didn't see obvious explanations for.

src/DetailsView/components/command-bar-buttons-menu.scss

src/DetailsView/components/command-bar-buttons-menu.tsx

src/DetailsView/components/save-assessment-button.tsx

src/DetailsView/components/save-assessment-button-factory.tsx

madalynrose · 2024-09-06T15:25:50Z

src/common/components/collapsible-component.tsx

                    onClick={this.onClick}
                    aria-expanded={showContent}
                >
+                    {showContent ? (


It might be better to keep the established pattern of calculating values outside of the return statement.

Hi @madalynrose agree, moved out of return , please re-review

src/tests/unit/tests/DetailsView/components/details-view-command-bar.test.tsx

madalynrose · 2024-09-06T18:10:30Z

src/tests/unit/tests/DetailsView/components/failure-instance-panel-control.test.tsx

@@ -112,7 +112,7 @@ describe('FailureInstancePanelControlTest', () => {
            'ActionButton',
            'DefaultButton',
        ]);
-        useOriginalReactElements('@fluentui/react-components', ['Link', 'Button']);
+        useOriginalReactElements('@fluentui/react-components', ['Link']);


Why are we changing strategy from fully rendering the Button and using a mocked component instead?

HI @madalynrose made changes, please re-review

madalynrose · 2024-09-06T18:18:44Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

+    const userConfigMessageCreatorMock: IMock<UserConfigMessageCreator> =
+        Mock.ofType(UserConfigMessageCreator);
+    const assessmentActionMessageCreatorMock: IMock<AssessmentActionMessageCreator> =
+        Mock.ofType<AssessmentActionMessageCreator>();
+    const handleSaveAssesmentButtonClickMock =
+        Mock.ofType<(event: React.MouseEvent<any>) => void>();
+    const userConfigurationStoreData: UserConfigurationStoreData = {
+        showSaveAssessmentDialog: true,
+    } as UserConfigurationStoreData;
+    const propsStub: SaveAssessmentButtonProps = {
+        deps: {
+            getAssessmentActionMessageCreator: () => assessmentActionMessageCreatorMock.object,
+            userConfigMessageCreator: userConfigMessageCreatorMock.object,
+        },
+        download: 'download',
+        href: 'url',
+        userConfigurationStoreData,
+        handleSaveAssesmentButtonClick: handleSaveAssesmentButtonClickMock.object,
+    };


Why is this no longer in a beforeEach block? It's good to make sure all of this stuff is reset before each test.

Hi @madalynrose updated and retested, please re-review

madalynrose · 2024-09-06T19:03:14Z

src/tests/unit/tests/DetailsView/components/save-assessment-dialog.test.tsx

@@ -0,0 +1,82 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


Why are there fewer tests than when these tests lived in SaveAssessmentButton?

Hi @madalynrose as we moved out SaveAssessmentDialog from save-assessment-button, hence dialog related tests and save assessment button tests are separated.

yes but it appears that there are fewer total now. I compared across both files.

src/tests/unit/tests/DetailsView/components/start-over-dropdown.test.tsx

madalynrose · 2024-09-09T15:20:54Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

-                    Times.atLeastOnce(),
-                );
-            });
-            it('dialog is hidden (dismissed) when "got it" button is clicked', async () => {


this test is missing

madalynrose · 2024-09-09T15:21:10Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

    });

-    describe('on dialog disabled', () => {


this test is missing

madalynrose · 2024-09-09T15:22:16Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

-            fireEvent.click(wrapper.getByRole('link'));
-        });
-
-        it('saves assessment without dialog (dialog is hidden)', () => {


this test is missing

madalynrose · 2024-09-09T15:23:06Z

src/tests/unit/tests/DetailsView/components/save-assessment-button.test.tsx

-            expect(getMockComponentClassPropsForCall(Dialog).hidden).toEqual(true);
-        });
-
-        it('should call saveAssessment on click', async () => {


this test is missing

v-rakeshsh and others added 18 commits June 21, 2024 11:41

action button initial migration

803dadb

Merge branch 'fluent-ui-v8-migration-main' of https://github.com/micr…

d858b39

…osoft/accessibility-insights-web into v-rakesh/actionbutton-migration

submenu changes

a36f0c2

submenu changes

24b7caf

submenu changes

b5af799

test case fix

f216ea9

changes in test files

f990b9a

latest test cases fixes

0df681f

latest test cases fixes

255638f

latest test cases fixes

46f7535

Removed comments from main files, numbers for testing purpose, consol…

54266d7

…es, format & lint fixes

chore: Added resolutions in package.json file to resolve CVE-2024-37890…

4859ff6

… CG Issue

latest test cases fixes

4049316

latest test cases fixes

4e8bd07

latest test cases fixes

6bffc0b

latest test cases fixes

2a1bd21

UI changes fixes

8c920ad

removed unwanted code

1a00b3c

v-rakeshsh requested a review from a team as a code owner July 30, 2024 06:45

v-rakeshsh marked this pull request as draft July 30, 2024 06:45

v-rakeshsh changed the base branch from main to fluent-ui-v9-migration-main July 30, 2024 11:45

v-rakeshsh and others added 9 commits August 1, 2024 10:21

latest test cases fixes

2311cf9

test cases fixes and code refactor

5e06d7f

lint issue fix and report e2e fix

9cfb513

test cases fixes

95d65aa

resolve conflicts

bbf8f22

test cases fix

7314e05

test cases fix

5814a4e

test cases fix

03ed64d

test cases and lint fixes

0feb778

v-rakeshsh and others added 19 commits August 23, 2024 12:06

fixiing focus issue

c37d16d

fixiing focus issue

0208c04

Removed unwanted comments, solved deprecated warnings files

f85fc99

lint and format fix

d8911f3

updated snapshots and test files

f8ed166

lint issue-fix

6ed52d2

fix e2e test failures

973ed6c

PR comments & e2e test snap update

fe6c22b

Build error fix

f3d8084

Changes for PR check errors

d3da3e6

fixing focus issues

b2e8510

fixing focus issues

9d6f6c9

fixing focus issues

2f8e72e

fix build issues

0d922b7

fix lint issues

ee88b74

fix lint issues

14a8e0c

fix lint issues

1ac70f9

retaining iButton

9877185

retaining iButton

9463f5d

v-viyada marked this pull request as ready for review August 29, 2024 16:42

v-viyada reviewed Aug 29, 2024

View reviewed changes

[initState Outdated Show resolved Hide resolved

v-rakeshsh added 4 commits August 30, 2024 11:21

delete initState file

9c83758

small changes

84e1794

small changes

968bf97

small changes

38346a6

madalynrose requested changes Sep 6, 2024

View reviewed changes

v-rakeshsh added 3 commits September 9, 2024 19:06

chore: review comments fix

d4982b0

chore: review comments fix

602348a

chore: review comments fix

0cb48d5

madalynrose requested changes Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fluentUI-migration): ActiontButton fluentui v8 to v9 migration #7401

feat(fluentUI-migration): ActiontButton fluentui v8 to v9 migration #7401

v-rakeshsh commented Jul 30, 2024 •

edited

Loading

madalynrose left a comment

madalynrose Sep 6, 2024

v-rakeshsh Sep 9, 2024

madalynrose Sep 6, 2024

v-rakeshsh Sep 9, 2024

madalynrose Sep 6, 2024

v-rakeshsh Sep 9, 2024

madalynrose Sep 6, 2024

v-rakeshsh Sep 9, 2024

madalynrose Sep 9, 2024

madalynrose Sep 9, 2024

madalynrose Sep 9, 2024

madalynrose Sep 9, 2024

madalynrose Sep 9, 2024

		@@ -0,0 +1,82 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

feat(fluentUI-migration): ActiontButton fluentui v8 to v9 migration #7401

Are you sure you want to change the base?

feat(fluentUI-migration): ActiontButton fluentui v8 to v9 migration #7401

Conversation

v-rakeshsh commented Jul 30, 2024 • edited Loading

Details

Styling Improvements:

Component Enhancements:

Motivation

Context

Pull request checklist

madalynrose 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

v-rakeshsh commented Jul 30, 2024 •

edited

Loading