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 hard-coded values for spacing with variables specified on _spacing.pcss #25070

Closed
luixxiul opened this issue Apr 9, 2023 · 2 comments
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Tolerable Low/no impact on users T-Enhancement

Comments

@luixxiul
Copy link

luixxiul commented Apr 9, 2023

Your use case

What would you like to do?

Replace hard-coded values for spacing with variables specified on _spacing.pcss.

Why would you like to do it?

Because the style guide suggests you to use a variable for a property related to spacing. It should also help us to maintain the themes' consistency.

How would you like to achieve it?

Search padding and margin values and replace them.

Have you considered any alternatives?

No response

Additional context

This should be conducted along with matrix-org/matrix-react-sdk#10552 for #21656.

@NSV1991
Copy link

NSV1991 commented Apr 11, 2023

@luixxiul I like to work on this enhancement.
What is the intended scope for the changes for this enhancement?
I mean which module we should target first?

@weeman1337 weeman1337 added A-Developer-Experience S-Minor Impairs non-critical functionality or suitable workarounds exist O-Occasional Affects or can be seen by some users regularly or most users rarely S-Tolerable Low/no impact on users and removed S-Minor Impairs non-critical functionality or suitable workarounds exist labels Apr 20, 2023
@richvdh
Copy link
Member

richvdh commented Apr 21, 2023

So, I discussed this with the rest of the team, with conclusions as follows:

Let's not do this: it seems like a lot of work for both authors and reviewers, for no tangible benefit.

There is debate over whether the current $spacing- variables offer any value at all (see matrix-org/matrix-react-sdk#10686), but even if we do want to use them going forward, we don't think a wholesale replacement of existing values offers any value at all.

In the longer term, we'd like to use components based on our "Compound" design system, which will use semantic variables rather than hardcoding particular px values, but that requires design input and updating legacy components is not a priority for the design team.

@richvdh richvdh closed this as completed Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Tolerable Low/no impact on users T-Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants