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 selected cluster type ui in Checks catalog view #3229

Merged
merged 2 commits into from
Jan 22, 2025

Conversation

EMaksy
Copy link
Member

@EMaksy EMaksy commented Jan 14, 2025

Description

This pr will restore the previous ui behaviour in the checks catalog view. When selecting a cluster type in the checks catalog the green svg icon is not rendered.
With the latest changes in #3210 we now not only pass a string as value, which was handled by the built in headless ui selected boolean, but also an object { type, hanaScenario } . Check out the official docs https://headlessui.com/v1/react/listbox#listbox-option
With using the lodash isEqual we now use a "deep comparison" to ensure accurate evaluation of selected options.

For this we use the "by" value provided by Listbox:
You can also pass your own comparison function to the by prop if you'd like complete control over how objects are compared
Source: https://headlessui.com/v1/react/listbox#binding-objects-as-values

Demo:

Screencast.From.2025-01-14.17-16-08.mp4

Test

Enriched existing e2e test to test that the SVG is rendered

@EMaksy EMaksy added bug Something isn't working javascript Pull requests that update Javascript code env Create an ephimeral environment for the pr branch labels Jan 14, 2025
@EMaksy EMaksy requested a review from arbulu89 January 14, 2025 16:53
@EMaksy EMaksy self-assigned this Jan 14, 2025
@EMaksy EMaksy force-pushed the add_missing_icon_cluster_types branch from 97146d2 to 94f552c Compare January 14, 2025 16:55
@EMaksy EMaksy requested a review from CDimonaco January 14, 2025 17:02
@EMaksy EMaksy changed the title Fix selected cluster type ui Fix selected cluster type ui in Checks catalog view Jan 14, 2025
@EMaksy EMaksy force-pushed the add_missing_icon_cluster_types branch from 94f552c to 7543726 Compare January 17, 2025 16:14
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Sweet!

@EMaksy EMaksy force-pushed the add_missing_icon_cluster_types branch from eb332f5 to b812f47 Compare January 21, 2025 13:48
@EMaksy EMaksy marked this pull request as ready for review January 21, 2025 15:23
@EMaksy EMaksy requested a review from nelsonkopliku January 22, 2025 09:26
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Thanks @EMaksy!

@EMaksy EMaksy force-pushed the add_missing_icon_cluster_types branch from b812f47 to d79e746 Compare January 22, 2025 09:37
@EMaksy EMaksy merged commit 99f0240 into main Jan 22, 2025
38 checks passed
@EMaksy EMaksy deleted the add_missing_icon_cluster_types branch January 22, 2025 09:49
@@ -93,12 +93,15 @@ context('Checks catalog', () => {
});

describe('Filtering', () => {
const selectedIconSelector =
Copy link
Contributor

Choose a reason for hiding this comment

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

question @EMaksy :
why did you create this constant and put in all the array entries?
I mean, you could simply have the constant and use it in the test, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very fair point, when working on this first i thought the ui selector would be different, but nope.
You are right we can just test directly, will create a follow up pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working env Create an ephimeral environment for the pr branch javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

3 participants