Conversation
There was a problem hiding this comment.
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)
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
- 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)
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
- In SCSS, use calc() for calculations involving mixed units (e.g., rem and px) as Sass cannot resolve them at compile time.
52d1ad7 to
c0b1396
Compare
| @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)); |
There was a problem hiding this comment.
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
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: