Skip to content

refactor(slider): use relative units for si-slider#2022

Open
spliffone wants to merge 1 commit intomainfrom
feat/slider-relative-sizing
Open

refactor(slider): use relative units for si-slider#2022
spliffone wants to merge 1 commit intomainfrom
feat/slider-relative-sizing

Conversation

@spliffone
Copy link
Copy Markdown
Member

@spliffone spliffone commented May 6, 2026

@spliffone spliffone requested review from a team as code owners May 6, 2026 16:28
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the slider component's layout from flexbox to CSS grid and converts several dimension variables from pixels to relative rem units. Feedback suggests ensuring all dimension variables use consistent rem units for better adherence to the project's objectives, removing unused variables, and utilizing the $button-width variable within the grid template to improve maintainability and layout consistency.

I am having trouble creating individual review comments. Click here to see my feedback.

projects/element-ng/slider/si-slider.component.scss (3-8)

medium

To fully adhere to the PR's objective of using relative units, all dimension variables should be converted to rem. Additionally, $track-height and $thumb-size appear to be unused in this stylesheet; consider removing them if they are not needed for the component's styling.

$button-width: 2rem;
$button-height: 2rem;
$button-gap: 0.25rem;

$track-height: 0.25rem;
$thumb-size: 1.5rem;
References
  1. When performing calculations in SCSS involving variables, ensure that the units of the variables are consistent (e.g., all pixels) if the calculation is to be resolved at compile time by Sass. If units are mixed (e.g., rem and px), use the CSS calc() function to ensure the browser handles unit resolution correctly at runtime.

projects/element-ng/slider/si-slider.component.scss (17)

medium

The $button-width variable should be utilized in the grid-template for the button columns. This ensures the layout is driven by the defined variables rather than relying on auto sizing, improving consistency and maintainability.

  grid-template: auto $button-height / $button-width 1fr $button-width;

@dr-itz
Copy link
Copy Markdown
Member

dr-itz commented May 6, 2026

Because @github is broken again I cannot comment in the code...

This: button dimensions should be aligned with our circle buttons I think:

$button-width: calc(1lh + 2 * map.get(variables.$spacers, 4));
$button-height: calc(1lh + 2 * map.get(variables.$spacers, 4));

@dr-itz
Copy link
Copy Markdown
Member

dr-itz commented May 6, 2026

i.e. @github messing up..sometime with error, sometimes silently dropping the comment. Nice.

Screenshot 2026-05-06 at 18 49 15

@spliffone spliffone force-pushed the feat/slider-relative-sizing branch from c1bfa17 to 72a214f Compare May 7, 2026 04:58
Comment on lines 115 to 118
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess these need some rem/calc magic too?

@spliffone spliffone force-pushed the feat/slider-relative-sizing branch from 37aa972 to dc00560 Compare May 7, 2026 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants