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

Reduce height of toggle on expanded view source event #9067

Closed
wants to merge 5 commits into from
Closed

Reduce height of toggle on expanded view source event #9067

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75aa753
Reduce height of toggle on expanded view source event
luixxiul Jul 17, 2022
2ca58c9
Fix expand toggle compression
luixxiul Jul 17, 2022
de2de88
get -> contains
luixxiul Jul 21, 2022
fca04ac
Merge remote-tracking branch 'upstream/develop' into ViewSourceEvent2
luixxiul Jul 21, 2022
e7cf034
Merge tests
luixxiul Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions cypress/e2e/14-timeline/timeline.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,37 @@ describe("Timeline", () => {
});
});

it("should set size and position to event toggle on expanded view source event", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can re-organise the tests to have one test that goes through all the event source bits. In this particular place I am thinking of merging it with https://github.com/matrix-org/matrix-react-sdk/blob/develop/cypress/e2e/14-timeline/timeline.spec.ts#L154

While it is nice to separate every case it may take too long in the future when we are adding more and more tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably I just don't know what I am doing (ie. ignorance), but I saw this part of the docs: https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/cypress.md#good-test-hygiene

  1. Test a well-isolated unit of functionality. The more specific, the easier it will be to tell what's wrong when they fail.
  2. Don't depend on state from other tests: any given test should be able to run in isolation.

Is it fine to merge several tests into one, if the same function is tested?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably I just don't know what I am doing (ie. ignorance), but I saw this part of the docs: https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/cypress.md#good-test-hygiene

Good point. Sorry for confusing you. Going to discuss that part of the docs.

Is it fine to merge several tests into one, if the same function is tested?

I would say yes as long as it tests the same bits and it shares the steps getting there. Here it is all about the event source view. On the other hand I would not mix up „sign up“ and „event source view“ tests for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say yes as long as it tests the same bits and it shares the steps getting there. Here it is all about the event source view. On the other hand I would not mix up „sign up“ and „event source view“ tests for example.

That totally makes sense. It would be perfect if the policy would be available as documentation.

sendEvent(roomId);
cy.visit("/#/room/" + roomId);
cy.setSettingValue("showHiddenEventsInTimeline", null, SettingLevel.DEVICE, true);
cy.contains(".mx_RoomView_body .mx_GenericEventListSummary .mx_GenericEventListSummary_summary",
"created and configured the room.");

// Edit message
cy.get(".mx_RoomView_body .mx_EventTile").contains(".mx_EventTile_line", "Message").within(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If there can be multiple items (like I would expect for tiles) I suggest to always use contains instead of get.
Otherwise Cypress runs the contains test on the first result of cy.get(".mx_RoomView_body .mx_EventTile").

Suggested change
cy.get(".mx_RoomView_body .mx_EventTile").contains(".mx_EventTile_line", "Message").within(() => {
cy.contains(".mx_RoomView_body .mx_EventTile .mx_EventTile_line", "Message").within(() => {

Copy link
Contributor Author

@luixxiul luixxiul Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was following other existing examples on threads.spec.ts without knowing what I was doing.

Thanks for the info!

Copy link
Contributor

Choose a reason for hiding this comment

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

I was following other existing examples on threads.spec.ts.

Thanks for the info!

Most of the time you are lucky that only a single element matches.
But imho the tests are more reliable when using cy.contains.

cy.get('[aria-label="Edit"]').click({ force: true }); // Cypress has no ability to hover
cy.get(".mx_BasicMessageComposer_input").type("Edit{enter}");
});
cy.get(".mx_RoomView_body .mx_EventTile").contains(".mx_EventTile[data-scroll-tokens]", "MessageEdit");

// Expand
cy.get(".mx_EventTile.mx_EventTile_info:first-of-type .mx_EventTile_line").realHover()
.get(".mx_ViewSourceEvent_toggle").click();

// Exclude timestamp and event content from snapshot
const percyCSS = ".mx_RoomView_body .mx_EventTile_info .mx_MessageTimestamp, " +
".mx_EventTile_content.mx_ViewSourceEvent pre { visibility: hidden !important; }";

// Check size and position of toggle on expanded view source event
// cf. _ViewSourceEvent.pcss
cy.get(".mx_ViewSourceEvent.mx_EventTile_content.mx_ViewSourceEvent_expanded").realHover()
.percySnapshot("ViewSourceEvent_toggle", { percyCSS })
.get(".mx_ViewSourceEvent_toggle")
.should('have.css', 'height', '12px') // --ViewSourceEvent_toggle-size
.should('have.css', 'align-self', 'flex-end');
});

it("should create and configure a room on IRC layout", () => {
cy.visit("/#/room/" + roomId);
cy.setSettingValue("layout", null, SettingLevel.DEVICE, Layout.IRC);
Expand Down
9 changes: 7 additions & 2 deletions res/css/views/messages/_ViewSourceEvent.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,24 @@ limitations under the License.
}

.mx_ViewSourceEvent_toggle {
--ViewSourceEvent_toggle-size: 12px;

visibility: hidden;
// override styles from AccessibleButton
border-radius: 0;
// icon
mask-repeat: no-repeat;
mask-position: 0 center;
mask-size: auto 12px;
width: 12px;
mask-size: auto var(--ViewSourceEvent_toggle-size);
width: var(--ViewSourceEvent_toggle-size);
min-width: var(--ViewSourceEvent_toggle-size);
background-color: $accent;
mask-image: url("$(res)/img/element-icons/maximise-expand.svg");
}

&.mx_ViewSourceEvent_expanded .mx_ViewSourceEvent_toggle {
align-self: flex-end;
height: var(--ViewSourceEvent_toggle-size);
mask-position: 0 bottom;
margin-bottom: 5px;
mask-image: url("$(res)/img/element-icons/minimise-collapse.svg");
Expand Down