Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace Sass variables with custom properties on _font-weights.pcss #10554

Merged
merged 5 commits into from
Apr 21, 2023
Merged

Replace Sass variables with custom properties on _font-weights.pcss #10554

merged 5 commits into from
Apr 21, 2023

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Apr 9, 2023

For element-hq/element-web#21656 and element-hq/element-web#25073

This PR intends to replace existing Sass variables on _font-weights.pcss and declarations with hard-coded value (font-weight: 600) with CSS custom properties, based on the style guide.

Currently there are two custom properties on _font-weights.pcss (--font-normal and --font-semi-bold). Custom properties for the other weight values are yet to be implemented.

The PR is based on the assumption that font-weight: 600 is always equal to font-weight: var(--font-semi-bold). Please correct me if I am wrong.

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 Apr 9, 2023
@luixxiul luixxiul changed the title Replace a font-weight variable on _font-weights.pcss Replace a Sass variable with a CSS custom property - _font-weights.pcss Apr 9, 2023
@luixxiul luixxiul changed the title Replace a Sass variable with a CSS custom property - _font-weights.pcss Replace a Sass variable and hard-coded values with a CSS custom property - _font-weights.pcss Apr 9, 2023
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul luixxiul changed the title Replace a Sass variable and hard-coded values with a CSS custom property - _font-weights.pcss Replace a Sass variable and hard-coded values with --font-semi-bold on _font-weights.pcss Apr 20, 2023
@luixxiul luixxiul changed the title Replace a Sass variable and hard-coded values with --font-semi-bold on _font-weights.pcss Replace Sass variables with custom properties on _font-weights.pcss Apr 20, 2023
@luixxiul luixxiul marked this pull request as ready for review April 20, 2023 04:48
@luixxiul luixxiul requested a review from a team as a code owner April 20, 2023 04:48
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

great, thanks!

@richvdh richvdh enabled auto-merge April 21, 2023 13:10
@luixxiul
Copy link
Contributor Author

Fixing the conflict..

auto-merge was automatically disabled April 21, 2023 13:17

Head branch was pushed to by a user without write access

@luixxiul luixxiul requested a review from richvdh April 21, 2023 13:19
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks like there's still a reference to $font-semi-bold somewhere which is breaking CI

@@ -27,12 +27,10 @@ $SpaceRoomViewInnerWidth: 428px;
font-weight: $font-semi-bold;
Copy link
Member

Choose a reason for hiding this comment

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

here, in fact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, addressed with 9e30d1b.

@luixxiul
Copy link
Contributor Author

This time it finally should be ok..

@luixxiul luixxiul requested a review from richvdh April 21, 2023 14:15
@richvdh richvdh added this pull request to the merge queue Apr 21, 2023
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Merged via the queue into matrix-org:develop with commit 92906c2 Apr 21, 2023
@luixxiul luixxiul deleted the css-variables-font-weights branch April 21, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

2 participants