Remove `mx_HeaderButtons` class and add a test #10713

luixxiul · 2023-04-26T14:22:53Z

Cherry-picked from #10495

This PR intends to remove mx_HeaderButtons class and add a test for the room header.

Thanks to #10628 mx_HeaderButtons class is no longer necessary as ARIA tabpanel role has been removed from the element with that class name, and it is possible to remove both the class and the style rule for it.

Here is the cropped screenshot of the DOM tree of mx_RoomHeader (mx_RoomHeader_wrapper) on this PR:

Please note that #10628 changed the buttons' ARIA role from tab to button and removed role="tabpanel" from the div element which had wrapped buttons with tab role, so we no longer need the wrapper. mx_RoomHeader_wrapper takes care of the placement of the all buttons inside it.

With this PR these Percy snapshots are added:

Room header
Room header - with a long room name
Room header - with a highlighted button
Room header - with a video room

Signed-off-by: Suguru Hirahara [email protected]

type: task

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

Now that mx_HeaderButtons class is no longer necessary as ARIA "tabpanel" role has been removed from it, it is possible to remove both the class and the style rule for it. Signed-off-by: Suguru Hirahara <[email protected]>

Signed-off-by: Suguru Hirahara <[email protected]>

alunturner

Hello. A few comments and some questions (mainly due to a bit of unfamiliarity on my side). The question about how to repeat a check for multiple buttons also applies to some of the later tests in this file.

Thanks for the contribution!

alunturner · 2023-04-28T07:57:03Z

cypress/e2e/room/room-header.spec.ts

+            cy.findByRole("button", { name: "Room options" }).should("be.visible");
+            cy.findByRole("button", { name: "Voice call" }).should("be.visible");
+            cy.findByRole("button", { name: "Video call" }).should("be.visible");
+            cy.findByRole("button", { name: "Search" }).should("be.visible");
+            cy.findByRole("button", { name: "Threads" }).should("be.visible");
+            cy.findByRole("button", { name: "Notifications" }).should("be.visible");
+            cy.findByRole("button", { name: "Room info" }).should("be.visible");


I'll preface this comment with saying I'm pretty new to cypress (and specifically the testing-library stuff with it).

To me, it seems that on line 40 we're already doing this test, aren't we? We're grabbing all the buttons, checking there are 7 of them, and I think checking that each thing with the role of button is visible (I could be misunderstanding how chaining a .should after a findAllBy* works though).

There may still be value in checking that we have these 7 specific buttons on the page though (ie checking the name of each button).

I've had a look to see if there's a neater way of doing this, nothing leaps out at me from the jest documentation. Perhaps something like:

const expectedButtonNames = [`Room options`...]; for (const name of expectedButtonNames) { cy.findByRole("button", { name}).should("be.visible"); }

Might make it a bit easier to read. What do you think?

Basically the object of this test is to ensure that just those seven (not six or eight) buttons are rendered by default. This can be achieved by combining findAllBy query with should(have.length) by counting the exact number of the all found buttons.

Each findByRole query does ensure that button whose name is specified exists, but it cannot ensure that there was not another button rendered besides them unexpectedly.

Based on your hint, b20ceec should optimize the readability and the test flow.

alunturner · 2023-04-28T07:58:38Z

cypress/e2e/room/room-header.spec.ts

+        });
+
+        cy.get(".mx_RoomHeader").percySnapshotElement("Room header", {
+            widths: [300, 600], // Magic numbers


Again I'm unfamiliar with percy too - what are these magic numbers for? Are they the widths the snapshots are performed at? How come you chose these values?

Values specified with widths are used to set custom widths of a snapshot of the chained element. The default values are 1024 and 1920 (both in pixel) specified here on .percy.yml.

On this case, Percy creates a snapshot of mx_RoomHeader (mind this is not the whole UI) in 300px and 600px. Similarly, this snapshot was captured in 320px and 640px, in order to emulate the narrow UI.

The custom values are indeed relevant only for Room header - with a long room name, since the object of the snapshot with overriding the default values should be to emulate the narrow UI and ensure that room name overflow is properly hidden with horizontal ellipsis. I'm going to remove unrelated widths settings.

Addressed with 2922c66.

alunturner · 2023-04-28T08:01:35Z

cypress/e2e/room/room-header.spec.ts

+            cy.get(".mx_RoomHeader_button")
+                .should("have.length", 3)
+                .should("be.visible")
+                .should("have.css", "height", "32px")
+                .should("have.css", "width", "32px");
+            cy.get(".mx_RightPanel_headerButton") // TODO: use the same class name
+                .should("have.length", 3)
+                .should("be.visible")
+                .should("have.css", "height", "32px")
+                .should("have.css", "width", "32px");


How come we have a test for 7 buttons earlier on, but we only check 6 here?

Because mx_RoomHeader_name (the room options button with dropdown menu on click) is also a button element.

This is the mx_RoomHeader_name:

I'm going to add a comment about it. It is indeed confusing.

Addressed with de2dd1c

alunturner · 2023-04-28T08:04:44Z

cypress/e2e/room/room-header.spec.ts

+
+            cy.get(".mx_RoomHeader").findByRole("button", { name: "Chat" }).click();
+
+            // Assert that the video is rnedered


Tiny typo here in rnedered

Thanks! Fixed with d7a700d

alunturner · 2023-04-28T08:09:20Z

cypress/e2e/room/room-header.spec.ts

+    it("should have a button highlighted by being clicked", () => {
+        cy.createRoom({ name: "Test Room" }).viewRoomByName("Test Room");
+
+        cy.findByRole("button", { name: "Room info" })


Would we want to test for all buttons here?

Currently only buttons which display content on the right panel by being clicked are highlighted; Thread, Notification, and Room Info, though I am not sure if this is intended or not.

8c9eee1 should address this.

luixxiul · 2023-04-28T09:25:47Z

Thanks for the initial review. I'm checking the code to reply to your feedback comments.

Signed-off-by: Suguru Hirahara <[email protected]>

luixxiul · 2023-04-28T13:11:55Z

cypress/e2e/room/room-header.spec.ts

+
+            // Assert the size of buttons on RoomHeader are specified and the buttons are not compressed
+            // Note these assertions do not check the size of mx_RoomHeader_name button
+            // TODO: merge the assertions by using the same class name


It will be achieved by #10495.

alunturner

This all looks good to me

luixxiul · 2023-05-05T16:06:11Z

Thanks for the review!

luixxiul · 2023-05-08T05:45:58Z

The Persy snapshots were taken as expected: https://percy.io/dfde73bd/matrix-react-sdk/builds/27192947/search?searchParam=Room%20header

luixxiul · 2023-05-08T05:47:49Z

https://percy.io/dfde73bd/matrix-react-sdk/builds/27192947/missing

This is because the snapshot was renamed to Room header - with a long room name by this PR.

github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Apr 26, 2023

github-actions bot deployed to Netlify April 26, 2023 14:27 View deployment

github-actions bot added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 26, 2023

github-actions bot deployed to Netlify April 26, 2023 17:34 View deployment

github-actions bot deployed to Netlify April 26, 2023 18:11 View deployment

github-actions bot deployed to Netlify April 27, 2023 05:05 View deployment

github-actions bot deployed to Netlify April 27, 2023 05:35 View deployment

github-actions bot deployed to Netlify April 27, 2023 09:32 View deployment

luixxiul added 2 commits April 27, 2023 10:11

Remove _HeaderButtons.pcss

8af7270

Now that mx_HeaderButtons class is no longer necessary as ARIA "tabpanel" role has been removed from it, it is possible to remove both the class and the style rule for it. Signed-off-by: Suguru Hirahara <[email protected]>

Tests: add room-header.spec.ts and edit the existing tests

5f3b148

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 27, 2023 10:21 View deployment

luixxiul marked this pull request as ready for review April 27, 2023 10:57

luixxiul requested a review from a team as a code owner April 27, 2023 10:57

luixxiul requested review from germain-gg and alunturner April 27, 2023 10:57

alunturner reviewed Apr 28, 2023

View reviewed changes

Edit snapshot widths settings

2922c66

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 28, 2023 10:16 View deployment

Fix typo

d7a700d

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 28, 2023 10:23 View deployment

Edit a comment

de2dd1c

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 28, 2023 11:32 View deployment

Optimize a test to check default buttons

b20ceec

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 28, 2023 11:55 View deployment

Check highlighted buttons

8c9eee1

Signed-off-by: Suguru Hirahara <[email protected]>

github-actions bot deployed to Netlify April 28, 2023 12:57 View deployment

Loop

831ee06

Signed-off-by: Suguru Hirahara <[email protected]>

luixxiul requested a review from alunturner April 28, 2023 13:07

github-actions bot deployed to Netlify April 28, 2023 13:08 View deployment

luixxiul commented Apr 28, 2023

View reviewed changes

alunturner approved these changes May 5, 2023

View reviewed changes

alunturner added this pull request to the merge queue May 5, 2023

Merged via the queue into matrix-org:develop with commit f6f5d44 May 5, 2023

luixxiul deleted the HeaderButtons2 branch May 5, 2023 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `mx_HeaderButtons` class and add a test #10713

Remove `mx_HeaderButtons` class and add a test #10713

luixxiul commented Apr 26, 2023 •

edited by github-actions bot

Loading

alunturner left a comment •

edited

Loading

alunturner Apr 28, 2023

luixxiul Apr 28, 2023

alunturner Apr 28, 2023

luixxiul Apr 28, 2023 •

edited

Loading

luixxiul Apr 28, 2023

alunturner Apr 28, 2023

luixxiul Apr 28, 2023

luixxiul Apr 28, 2023

luixxiul Apr 28, 2023

alunturner Apr 28, 2023

luixxiul Apr 28, 2023

alunturner Apr 28, 2023

luixxiul Apr 28, 2023

luixxiul commented Apr 28, 2023

luixxiul Apr 28, 2023

alunturner left a comment

luixxiul commented May 5, 2023

luixxiul commented May 8, 2023

luixxiul commented May 8, 2023


		cy.get(".mx_RoomHeader").findByRole("button", { name: "Chat" }).click();

		// Assert that the video is rnedered

Remove mx_HeaderButtons class and add a test #10713

Remove mx_HeaderButtons class and add a test #10713

Conversation

luixxiul commented Apr 26, 2023 • edited by github-actions bot Loading

Checklist

alunturner left a comment • edited Loading

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

luixxiul Apr 28, 2023 • edited Loading

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

luixxiul commented Apr 28, 2023

Choose a reason for hiding this comment

alunturner left a comment

Choose a reason for hiding this comment

luixxiul commented May 5, 2023

luixxiul commented May 8, 2023

luixxiul commented May 8, 2023

Remove `mx_HeaderButtons` class and add a test #10713

Remove `mx_HeaderButtons` class and add a test #10713

luixxiul commented Apr 26, 2023 •

edited by github-actions bot

Loading

alunturner left a comment •

edited

Loading

luixxiul Apr 28, 2023 •

edited

Loading