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

Fix --name-width on _IRCLayout.pcss#9064

Closed
luixxiul wants to merge 1 commit into
matrix-org:developfrom
luixxiul:GenericEventListSummary_IRC
Closed

Fix --name-width on _IRCLayout.pcss#9064
luixxiul wants to merge 1 commit into
matrix-org:developfrom
luixxiul:GenericEventListSummary_IRC

Conversation

@luixxiul
Copy link
Copy Markdown
Contributor

@luixxiul luixxiul commented Jul 16, 2022

This PR fixes the --name-width on _IRCLayout.pcss.

The name width --name-width was set to 70px on this line of #4531, while at the same time the default value was set to 80px on this line of the same PR. The reason is not clear, but since the latter value has been respected on the UI anyway, the former should be able to be changed to 80px.

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

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.

@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 Jul 16, 2022
// calc(var(--name-width) + var(--icon-width) + $MessageTimestamp_width + 3 * var(--right-padding));
// = 80 + 14 + 46($MessageTimestamp_width) + 3*5 = 155px
cy.get(".mx_GenericEventListSummary[data-layout=irc] > .mx_EventTile_line")
.should('have.css', 'padding-inline-start', '155px');
Copy link
Copy Markdown
Contributor Author

@luixxiul luixxiul Jul 16, 2022

Choose a reason for hiding this comment

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

Setting this value to 145px (--name-width: 70px) returns an error, therefore --name-width on _IRClayout.pcss should be changed from 70px to 80px, which is a default value on a new room.

Copy link
Copy Markdown
Contributor

@weeman1337 weeman1337 Jul 18, 2022

Choose a reason for hiding this comment

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

If only CSS properties are asserted without a screenshot this test should be a jest unit-test. You can ask for support on how to do this through the usual channels.

@luixxiul luixxiul marked this pull request as ready for review July 16, 2022 14:48
@luixxiul luixxiul requested a review from a team as a code owner July 16, 2022 14:48
// calc(var(--name-width) + var(--icon-width) + $MessageTimestamp_width + 3 * var(--right-padding));
// = 80 + 14 + 46($MessageTimestamp_width) + 3*5 = 155px
cy.get(".mx_GenericEventListSummary[data-layout=irc] > .mx_EventTile_line")
.should('have.css', 'padding-inline-start', '155px');
Copy link
Copy Markdown
Contributor

@weeman1337 weeman1337 Jul 18, 2022

Choose a reason for hiding this comment

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

If only CSS properties are asserted without a screenshot this test should be a jest unit-test. You can ask for support on how to do this through the usual channels.

@t3chguy
Copy link
Copy Markdown
Member

t3chguy commented Jul 18, 2022

If only CSS properties are asserted without a screenshot this test should be a jest unit-test. You can ask for support on how to do this through the usual channels.

Except Jest doesn't load CSS due to it being loaded by the Element Web layer, thus needing an end-to-end test given how we structure our components.

@weeman1337 weeman1337 self-requested a review July 18, 2022 12:30
Comment thread cypress/e2e/14-timeline/timeline.spec.ts Outdated
});

it("should create and configure a room on IRC layout", () => {
it("should configure a room on IRC layout", () => {
Copy link
Copy Markdown
Contributor Author

@luixxiul luixxiul Jul 19, 2022

Choose a reason for hiding this comment

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

Because this test does not create a room.

// calc(var(--name-width) + var(--icon-width) + $MessageTimestamp_width + 3 * var(--right-padding));
// = 80 + 14 + 46($MessageTimestamp_width) + 3*5 = 155px
.should('have.css', 'padding-inline-start', '155px');
cy.percySnapshot("EventTile_line of GenericEventListSummary");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the same snapshot as in line 153, I would say. One should be enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks for pointing out.

@weeman1337
Copy link
Copy Markdown
Contributor

Hi @luixxiul can you please update your branch and resolve the conflicts?

@weeman1337
Copy link
Copy Markdown
Contributor

Hmm 🤔 They are looking both wrong

image

Were the screenshots taken too early, when the layout is still settling down?

@luixxiul
Copy link
Copy Markdown
Contributor Author

luixxiul commented Aug 2, 2022

@weeman1337
Copy link
Copy Markdown
Contributor

That is not what I mean. When looking at the actual app everything is bottom-aligned:

image

Both screenshots look different.

@luixxiul
Copy link
Copy Markdown
Contributor Author

luixxiul commented Aug 2, 2022

Ah I see, let me check.

…n Settings.tsx

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul luixxiul changed the title Add test to check inline start padding of EventTile_line of GenericEventListSummary on IRC layout Change --name-width on _IRCLayout.pcss based on Settings.tsx Mar 5, 2023
@luixxiul luixxiul changed the title Change --name-width on _IRCLayout.pcss based on Settings.tsx Fix --name-width on _IRCLayout.pcss Mar 5, 2023
@luixxiul luixxiul closed this Mar 5, 2023
@luixxiul luixxiul deleted the GenericEventListSummary_IRC branch March 5, 2023 06:36
@luixxiul
Copy link
Copy Markdown
Contributor Author

luixxiul commented Mar 5, 2023

The test has been already implemented with another PR. The issue related to the --name-width still exists, so I opened a PR for it as the main goal of this PR was to implement the test.

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

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

Development

Successfully merging this pull request may close these issues.

3 participants