Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add a basic E2E test for event tiles #10306

Merged
merged 11 commits into from
Mar 17, 2023
Merged

Add a basic E2E test for event tiles #10306

merged 11 commits into from
Mar 17, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 7, 2023 •
edited by github-actions bot
Loading

This PR intends to add E2E test to check EventTiles. Initially this test is to include a task to check declarations which are either difficult to detect with a screenshot test or specified with :not([data-layout=bubble]) pseudo class, in order to make sure that spacing value between continued event tiles is zero, before eventually removing this !important flag, for example. Other test scenarios such as bubble of messages sent by other users, deleted messages, etc are to be implemented in future.

Required for #9033

type: task

Signed-off-by: Suguru Hirahara luixxiul@users.noreply.github.com

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.

@github-actions github-actions bot added Z-Community-PR Issue is solved by a community member's PR T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Mar 7, 2023
@luixxiul
Add a test for event tiles on modern layout
68ec634 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Add a test for a normal EventTile
45ba532 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Check line-height of mx_EventTile_line
cc07d3a 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Check the declarations of event tiles on compact modern layout
802ab76 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Edit the test
095fdcb 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Add a test for tiles on bubble layout
8635088 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Add a test for tiles on IRC layout
b64af28 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul luixxiul changed the title Add a basic test for event tiles on modern layout Add a basic E2E test for event tiles Mar 15, 2023
luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
cy.get(".mx_EventTile")
.should("have.css", "max-width", "100%")
.should("have.css", "clear", "both")
.should("have.css", "position", "relative");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To confirm the declarations below (except padding and font size) are kept applied:

matrix-react-sdk/res/css/views/rooms/_EventTile.pcss

Lines 481 to 487 in 2344eaa

.mx_EventTile:not([data-layout="bubble"]) {
max-width: 100%;
clear: both;
padding-top: 18px;
font-size: $font-14px;
position: relative;
}

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
.should("exist")
.should("have.css", "max-width", "100%")
.should("have.css", "clear", "both")
.should("have.css", "position", "relative");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: https://github.com/matrix-org/matrix-react-sdk/pull/10306/files#r1138080152

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
cy.get(".mx_EventTile")
.should("have.css", "max-width", "none")
.should("have.css", "clear", "both")
.should("have.css", "position", "relative");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_EventBubbleTile.pcss

Lines 42 to 52 in 2344eaa

.mx_EventTile[data-layout="bubble"] {
--EventTile_bubble_line-margin-inline-start: -9px;
--EventTile_bubble_line-margin-inline-end: -12px;
--EventTile_bubble_gap-inline: 5px;
position: relative;
/* Other half of the gutter is provided by margin-bottom on the last tile
of the section */
margin-top: calc(var(--gutterSize) / 2);
margin-left: var(--EventTile_bubble-margin-inline-start);
font-size: $font-14px;

Checking default values as well.

@luixxiul
Remove .should('exist')
5b6da62 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
.should("have.css", "position", "relative");

// Check that block start padding of the second message is not overridden
cy.get(".mx_EventTile.mx_EventTile_continuation").should("have.css", "margin-block-start", "2px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_EventTile.pcss

Lines 470 to 472 in 2344eaa

&.mx_EventTile_continuation {
margin-top: 2px;
}

@luixxiul
Check the same declarations for continued EventTile on both IRC and m…
6f9f254 
…odern layout

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts

// Check mx_EventTile_continuation
// Block start padding of the second message should not be overridden
cy.get(".mx_EventTile_continuation").should("have.css", "padding-block-start", "0px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

matrix-react-sdk/res/css/views/rooms/_EventTile.pcss

Lines 405 to 407 in 2344eaa

&.mx_EventTile_continuation {
padding-top: 0px !important;
}

Ensuring zero block padding is applied without !important flag.

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts

// Check mx_EventTile_continuation
// Block start padding of the second message should not be overridden
cy.get(".mx_EventTile_continuation").should("have.css", "padding-block-start", "0px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_IRCLayout.pcss

Line 44 in 2344eaa

padding-top: 0;

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
cy.get(".mx_EventTile_line").should("have.css", "clear", "both");
})
// Check that zero block padding is set
.should("have.css", "padding-block-start", "0px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above: https://github.com/matrix-org/matrix-react-sdk/pull/10306/files#r1138094080

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
// Check mx_EventTile_continuation
// Block start padding of the second message should not be overridden
cy.get(".mx_EventTile_continuation").should("have.css", "padding-block-start", "0px");
cy.get(".mx_EventTile_continuation .mx_EventTile_line").should("have.css", "clear", "both");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_EventTile.pcss

Lines 239 to 241 in 2344eaa

&.mx_EventTile_continuation .mx_EventTile_line {
clear: both;
}

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
// Check mx_EventTile_continuation
// Block start padding of the second message should not be overridden
cy.get(".mx_EventTile_continuation").should("have.css", "padding-block-start", "0px");
cy.get(".mx_EventTile_continuation .mx_EventTile_line").should("have.css", "clear", "both");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_EventTile.pcss

Lines 239 to 241 in 2344eaa

&.mx_EventTile_continuation .mx_EventTile_line {
clear: both;
}

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
cy.get(".mx_EventTile_last")
.within(() => {
// calc(var(--gutterSize) - 1px)
cy.get(".mx_EventTile_line").should("have.css", "padding-block-start", "10px");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See:

matrix-react-sdk/res/css/views/rooms/_EventBubbleTile.pcss

Lines 345 to 349 in 2344eaa

&:not(.mx_EventTile_noBubble) .mx_EventTile_line:not(.mx_EventTile_mediaLine) {
/* make the top and bottom padding 1px smaller so that we can pad
.mx_EventTile_content by 1px */
/* to avoid anti-zalgo cutting off our larger than text emojis. */
padding: calc(var(--gutterSize) - 1px);

This messy selector with the double negation should be simplified eventually.

luixxiul
luixxiul commented Mar 16, 2023
View reviewed changes
cypress/e2e/timeline/timeline.spec.ts
// calc(var(--gutterSize) - 1px)
cy.get(".mx_EventTile_line").should("have.css", "padding-block-start", "10px");
})
.should("have.css", "margin-block-start", "2px"); // The last bubble is also a continued one
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above: https://github.com/matrix-org/matrix-react-sdk/pull/10306/files#r1138088543

@luixxiul
Merge the tests to speed up
a0aef75 
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul luixxiul marked this pull request as ready for review March 16, 2023 05:10
@luixxiul luixxiul requested a review from a team as a code owner March 16, 2023 05:10
weeman1337
weeman1337 approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@weeman1337 weeman1337 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test @luixxiul

@weeman1337 weeman1337 merged commit c9e7aaf into matrix-org:develop Mar 17, 2023
@luixxiul luixxiul deleted the test-continued-event-tile branch March 17, 2023 07:50
@luixxiul
Copy link
Contributor Author

Thanks for the quick review!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@weeman1337 weeman1337 weeman1337 approved these changes

@alunturner alunturner Awaiting requested review from alunturner alunturner is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luixxiul @weeman1337