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

🐛 trying to fix data grid bug, binded with gridStyles #7965

Closed
wants to merge 2 commits into from

Conversation

pamellix
Copy link

@pamellix pamellix commented Aug 15, 2024

Summary

I think that the bug can be fixed if we add a gridStyles to a deps array for useUpdateEffect for grid styles. Analysing the code I spotted that changes are commited if function onChange in gridStyles was called, that's why setDensity in reset button did not changed the style. So, I think that if some changes were made in gridStyles, then onChange should be called.

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes

@pamellix pamellix requested a review from a team as a code owner August 15, 2024 00:05
Copy link

cla-checker-service bot commented Aug 15, 2024

❌ Author of the following commits did not sign a Contributor Agreement:
17e37c0, e64f31b

Please, read and sign the above mentioned agreement if you want to contribute to this project

Copy link

👋 Since this is a community submitted pull request, a Buildkite build has not been started automatically. Would an Elastic organization member please verify the contents of this pull request and kick off a build manually?

@cee-chen
Copy link
Contributor

cee-chen commented Aug 30, 2024

This doesn't solve #7962 unfortunately. There's a lot more at play here in this logic which involves figuring out which updates need to take precedence (dev styles vs user styles). If we're going to start listening for updates to gridStyles from developers, we also need to not fire onChange since those should only fire on user style changes.

I'm going to go ahead and close this PR - it's likely the EUI team will pick up #7962, #7951, and #5593 simultaneously later.

@cee-chen cee-chen closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants