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

use spacing variables in components->views #10564

Closed

Conversation

NSV1991
Copy link
Contributor

@NSV1991 NSV1991 commented Apr 11, 2023

fixes element-hq/element-web#25070

Type: enhancement

Signed-off-by: Neeraj Vageele neerajvageele451@gmail.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)

Here's what your changelog entry will look like:

✨ Features

@NSV1991 NSV1991 requested a review from a team as a code owner April 11, 2023 11:16
@NSV1991 NSV1991 requested review from richvdh and artcodespace April 11, 2023 11:16
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Apr 11, 2023
@github-actions github-actions bot added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 11, 2023
res/css/_spacing.pcss Outdated Show resolved Hide resolved
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.

I'm not entirely convinced this is a useful change. Do we need to go through and replace every single margin, padding and position with a $spacing variable? What does that project actually achieve?

@gsouquet what are your thoughts?

@@ -67,7 +67,7 @@ limitations under the License.
}

.mx_BeaconStatus_label {
margin-bottom: 2px;
margin-bottom: $spacing-2;
Copy link
Member

Choose a reason for hiding this comment

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

I thought the whole point of the $spacing variables was to encourage people not to use values which are not a multiple of 4?

@luixxiul
Copy link
Contributor

luixxiul commented Apr 12, 2023

I'm not entirely convinced this is a useful change. Do we need to go through and replace every single margin, padding and position with a $spacing variable? What does that project actually achieve?

If needed, it would be appreciated to update this line on the style guide to make an internal agreement public.

@richvdh richvdh requested review from germain-gg and removed request for luixxiul April 12, 2023 09:43
@richvdh richvdh mentioned this pull request Apr 12, 2023
3 tasks
@artcodespace artcodespace removed their request for review April 20, 2023 14:52
@richvdh
Copy link
Member

richvdh commented Apr 21, 2023

@NSV1991: thank you for the contribution, but as per element-hq/element-web#25070: we don't think this is particularly useful.

@richvdh richvdh closed this Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements 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.

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