Skip to content

Refactor/theme/various#2023

Open
dr-itz wants to merge 4 commits intomainfrom
refactor/theme/various
Open

Refactor/theme/various#2023
dr-itz wants to merge 4 commits intomainfrom
refactor/theme/various

Conversation

@dr-itz
Copy link
Copy Markdown
Member

@dr-itz dr-itz commented May 6, 2026

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 hardcoded dimensions to use dynamic SCSS calculations with spacers across the avatar, circle status, and header account components. It also introduces layout adjustments for the side panel and badge text. Feedback was provided regarding the removal of !important and redundant overrides in the header account item, as well as maintaining consistent calculation formulas across components for better alignment with the design system.

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

projects/element-ng/application-header/si-header-account-item.component.scss (5)

medium

The use of !important should be avoided as it breaks the CSS cascade and makes future maintenance more difficult. Additionally, the calculated value (32px / 2rem) is identical to the default for the .small variant defined in si-avatar.component.scss. If the component already uses size="small", this override is redundant. If it must remain, please remove !important and use the consistent formula calc(1.25rem + 2 * spacer 3) to handle mixed units correctly at runtime.

  --avatar-size: calc(1.25rem + #{2 * map.get(spacers.$spacers, 3)});
References
  1. When performing calculations in SCSS involving variables with mixed units (e.g., rem and px), use the CSS calc() function to ensure the browser handles unit resolution correctly at runtime.

projects/element-ng/circle-status/si-circle-status.component.scss (7)

medium

The formula used to calculate the small size (1.5rem + 2 * spacer 2) is inconsistent with the one used for si-avatar.small (1.25rem + 2 * spacer 3), even though both result in 2rem. For better maintainability and alignment with the design system's logic, it is recommended to use a consistent base and spacer combination for the same logical size across components. Additionally, ensure mixed units are wrapped in calc() for proper browser resolution.

References
  1. In SCSS, use calc() for calculations involving mixed units (e.g., rem and px) as Sass cannot resolve them at compile time.

@dr-itz dr-itz force-pushed the refactor/theme/various branch from 52d1ad7 to c0b1396 Compare May 6, 2026 18:05
@dr-itz dr-itz marked this pull request as ready for review May 6, 2026 19:37
@dr-itz dr-itz requested review from a team as code owners May 6, 2026 19:37
@dr-itz dr-itz requested a review from spliffone May 6, 2026 19:38
@use '@siemens/element-theme/src/styles/variables';

$si-circle-status-size-regular: 2.5rem;
$si-circle-status-size-regular: calc(1.5rem + 2 * map.get(variables.$spacers, 4));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

could also go for 1.25rem + 20px here, i.e. the actual icon size + padding. (That would almost fit in datatable...kinda does at 24px)

similar for small below and in avatar

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.

1 participant