Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(aggrid): Improve styling for indeterminate checkbox to be displayed in AG Grid header #1755

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AndreasBerliner
Copy link
Collaborator

@AndreasBerliner AndreasBerliner commented Mar 7, 2025

💡 What is the current behavior?

  • When using AG Grid, the indeterminate state of the checkbox is not displayed in the grid header.
  • The hover state does not work for the check boxes in AG Grid.

🆕 What is the new behavior?

  • The indeterminate state is displayed in the grid header checkbox.
  • The hover state is now displayed for check boxes in AG Grid.

Important note: The solution contains a workaround using base64 encoded SVG images (background + icon). This needs to be replaced by a human readable solution with SVG images that also include the correct CSS variables for the icon color.

🏁 Checklist

A pull request can only be merged if all of these conditions are met (where applicable):

  • 🦮 Accessibility (a11y) features were implemented
  • 🗺️ Internationalization (i18n) - no hard coded strings
  • 📲 Responsiveness - components handle viewport changes and content overflow gracefully
  • 📄 Documentation was reviewed/updated (pnpm run docs)
  • 🧪 Unit tests were added/updated and pass (pnpm test)
  • 📸 Visual regression tests were added/updated and pass (Guide)
  • 🧐 Static code analysis passes (pnpm lint)
  • 🏗️ Successful compilation (pnpm build, changes pushed)

👨‍💻 Help & support

Copy link

changeset-bot bot commented Mar 7, 2025

🦋 Changeset detected

Latest commit: c62a90f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@siemens/ix-aggrid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@AndreasBerliner AndreasBerliner marked this pull request as ready for review March 7, 2025 11:17
@AndreasBerliner AndreasBerliner marked this pull request as draft March 7, 2025 14:48
Copy link

sonarqubecloud bot commented Mar 7, 2025

Copy link
Collaborator

@danielleroux danielleroux left a comment

Choose a reason for hiding this comment

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

We can spend here a visual regression test to test the indeterminate state

Comment on lines +78 to +79
//TODO: Replace base64 encoded image with svg and css variables
//Encoded image: <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"><rect x="2" y="7" width="12" height="2" fill="#000028"/></svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should remove the comments here. If you want to still have the todo create a jira ticket for that otherwise we will forget to remove this.

Copy link
Collaborator

@nuke-ellington nuke-ellington left a comment

Choose a reason for hiding this comment

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

We should add some VRT - maybe we can incorporate the header checkboxes into the existing ones?

'@siemens/ix-aggrid': patch
---

improve styling of aggrid for indeterminate checkbox to be displayed in grid header
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
improve styling of aggrid for indeterminate checkbox to be displayed in grid header
__AG Grid__: Improve styling of indeterminate checkbox to be displayed in grid header.

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.

3 participants