fix(core/checkbox, aggrid): added cursor to ix-checkbox#2496
fix(core/checkbox, aggrid): added cursor to ix-checkbox#2496GayatriK2002 wants to merge 7 commits intosiemens:mainfrom
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces hover and active states for checkboxes in the ag-grid package and adds a pointer cursor to the core checkbox component. The review feedback identifies potential layout shifts caused by removing borders in the checked state and a visual bug where hover styles on the checkmark mask could make it invisible. Additionally, the reviewer noted that axe-based accessibility tests are required for these component changes to comply with the repository's style guide.
| .ag-checkbox-input-wrapper.ag-checked, | ||
| .ag-checkbox-input-wrapper.ag-indeterminate { | ||
| position: relative; | ||
| background-color: var(--theme-checkbox-checked--color); | ||
| border: none; | ||
| } |
There was a problem hiding this comment.
Removing the border (border: none) while the unchecked state has a border will cause a layout shift when toggling the checkbox. It is recommended to maintain the border for visual consistency and to avoid size changes.
| .ag-checkbox-input-wrapper.ag-checked, | |
| .ag-checkbox-input-wrapper.ag-indeterminate { | |
| position: relative; | |
| background-color: var(--theme-checkbox-checked--color); | |
| border: none; | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked, | |
| .ag-checkbox-input-wrapper.ag-indeterminate { | |
| position: relative; | |
| background-color: var(--theme-checkbox-checked--color); | |
| border: var(--theme-checkbox--border-thickness) solid transparent; | |
| } |
References
- Avoid refactoring legacy tokens that are scheduled for future removal, even for consistency improvements.
There was a problem hiding this comment.
Updated CSS code for only unchecked hover and active states.
| .ag-checkbox-input-wrapper.ag-checked:hover, | ||
| .ag-checkbox-input-wrapper.ag-indeterminate:hover { | ||
| background-color: var(--theme-checkbox-checked--color--hover); | ||
| } | ||
|
|
||
| .ag-checkbox-input-wrapper.ag-checked:hover::before, | ||
| .ag-checkbox-input-wrapper.ag-indeterminate:hover::before { | ||
| background-color: var(--theme-checkbox-checked--background--hover); | ||
| } | ||
|
|
||
| .ag-checkbox-input-wrapper.ag-checked:active, | ||
| .ag-checkbox-input-wrapper.ag-indeterminate:active { | ||
| background-color: var(--theme-checkbox-checked--color--active); | ||
| } | ||
|
|
||
| .ag-checkbox-input-wrapper.ag-checked:active::before, | ||
| .ag-checkbox-input-wrapper.ag-indeterminate:active::before { | ||
| background-color: var(--theme-checkbox-checked--background--active); | ||
| } |
There was a problem hiding this comment.
Applying var(--theme-checkbox-checked--background--hover) to the ::before element (the checkmark mask) is incorrect as it would change the checkmark's color to the box's hover color, likely making it visually invisible. Additionally, the border should be maintained to prevent layout shifts.
| .ag-checkbox-input-wrapper.ag-checked:hover, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:hover { | |
| background-color: var(--theme-checkbox-checked--color--hover); | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked:hover::before, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:hover::before { | |
| background-color: var(--theme-checkbox-checked--background--hover); | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked:active, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:active { | |
| background-color: var(--theme-checkbox-checked--color--active); | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked:active::before, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:active::before { | |
| background-color: var(--theme-checkbox-checked--background--active); | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked:hover, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:hover { | |
| background-color: var(--theme-checkbox-checked--color--hover); | |
| border: var(--theme-checkbox--border-thickness) solid transparent; | |
| } | |
| .ag-checkbox-input-wrapper.ag-checked:active, | |
| .ag-checkbox-input-wrapper.ag-indeterminate:active { | |
| background-color: var(--theme-checkbox-checked--color--active); | |
| border: var(--theme-checkbox--border-thickness) solid transparent; | |
| } |
References
- Avoid refactoring legacy tokens that are scheduled for future removal, even for consistency improvements.
There was a problem hiding this comment.
Updated CSS code for only unchecked hover and active states.
|
|
||
| width: 100%; | ||
| height: 100%; | ||
| cursor: pointer; |
There was a problem hiding this comment.
The repository style guide requires axe-based component tests for component changes. This PR modifies the checkbox component's visual behavior but does not appear to include updated accessibility tests to ensure compliance.
References
- For component changes, check that accessibility coverage exists with an axe-based component test. (link)
There was a problem hiding this comment.
this CSS-only modification does not affect accessibility behavior
|
Fixed missing class="ix-form-control" in the input & label tag in the Angular standalone checkbox, checkbox-indeterminate preview example. |
|
1 participant
💡 What is the current behavior?
GitHub Issue Number: #
Jira Issue Number: 4092
🆕 What is the new behavior?
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support