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

Introduce CSS custom properties to _TopUnreadMessagesBar.pcss #10796

Closed
wants to merge 9 commits into from
Closed

Introduce CSS custom properties to _TopUnreadMessagesBar.pcss #10796

wants to merge 9 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented May 5, 2023

Closes element-hq/element-web#25282

This PR suggests to update _TopUnreadMessagesBar.pcss by introducing CSS custom properties, splitting structural definitions from their values. This way it will become possible to access and manipulate those values via TypeScript to change, for example, the border color and size, maintaining the style structure.

type: task

Image
TopUnreadMessageBar 2_2
mx_TopUnreadMessagesBar_scrollUp
cascading
2
mx_TopUnreadMessagesBar_markAsRead
cascading
2_1

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.

@luixxiul luixxiul marked this pull request as ready for review May 6, 2023 03:59
@luixxiul luixxiul requested a review from a team as a code owner May 6, 2023 03:59
@luixxiul luixxiul requested review from dbkr and richvdh May 6, 2023 03:59
width: var(--width);
height: var(--height);
border: var(--border);
border-radius: var(--borderRadius);
Copy link
Contributor Author

@luixxiul luixxiul May 6, 2023

Choose a reason for hiding this comment

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

Please note that using names which seem general for properties does not matter here, because the custom properties cascade (this is one of the main differences between the custom property and the preprocessor's variable). Even if --height, for example, is specified somewhere outside of this file / the selector, its value is not respected, because the value is explicitly specified below for each selector (--height: 38px and --height: 18px) and overridden. Comments for each variable are also simply redundant, as those variables are limited to those selectors, and it is obvious by themselves how they work. It would be like adding a comment to a declaration display: inherit that the value for this declaration is inherited, which we would not do.

&::before {
--maskImage: url("$(res)/img/cancel.svg");
--maskSize: 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.

--width: 18px and --height: 18px do not need to be repeated here, as they are inherited from mx_TopUnreadMessagesBar_markAsRead.

Comment on lines +40 to +41
.mx_TopUnreadMessagesBar_scrollUp,
.mx_TopUnreadMessagesBar_markAsRead {
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for nesting these where they were not before?

Copy link
Contributor Author

@luixxiul luixxiul May 10, 2023

Choose a reason for hiding this comment

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

To avoid repeating the same declarations, and to make the structure compact. Would you rather write like this?

    .mx_TopUnreadMessagesBar_scrollUp {
        background: var(--background);
        width: var(--width);
        height: var(--height);
        border: var(--border);
        border-radius: var(--borderRadius);

        --background: $background;
        --border: 1.3px solid $muted-fg-color;

        &::before {
            content: "";
            position: absolute;
            background: var(--background);
            width: var(--width);
            height: var(--height);
            mask-repeat: no-repeat;
            mask-position: center;
            mask-image: var(--maskImage);
            mask-size: var(--maskSize);

            --background: $muted-fg-color;
        }
    }

    .mx_TopUnreadMessagesBar_markAsRead {
        background: var(--background);
        width: var(--width);
        height: var(--height);
        border: var(--border);
        border-radius: var(--borderRadius);

        --background: $background;
        --border: 1.3px solid $muted-fg-color;

        &::before {
            content: "";
            position: absolute;
            background: var(--background);
            width: var(--width);
            height: var(--height);
            mask-repeat: no-repeat;
            mask-position: center;
            mask-image: var(--maskImage);
            mask-size: var(--maskSize);

            --background: $muted-fg-color;
        }
    }

It works, but it's not really efficient.

Copy link
Contributor Author

@luixxiul luixxiul May 10, 2023

Choose a reason for hiding this comment

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

Also, it seems to me that there has not been a specific reason either that those selectors were not combined, checking a commit like ac9902e#diff-21b2361c0ea031da22f3680c5912c17ccc3d86c5bad901dd533f004cde3efa32 of #2345 and 07348a6#diff-21b2361c0ea031da22f3680c5912c17ccc3d86c5bad901dd533f004cde3efa32 of #4159.

Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid I don't understand your answer. I think you are explaining why .mx_TopUnreadMessagesBar_scrollUp and .mx_TopUnreadMessagesBar_markAsRead are combined, but I didn't ask that. I asked why they are nested (inside .mx_TopUnreadMessagesBar).

@richvdh richvdh requested review from richvdh and removed request for richvdh May 11, 2023 10:01
@luixxiul luixxiul closed this Jun 1, 2023
@luixxiul luixxiul deleted the TopUnreadMessagesBar branch June 1, 2023 14:33
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.

Update _TopUnreadMessagesBar.pcss by introducing CSS custom properties
2 participants