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

Add ChecksSelection component #1577

Merged
merged 5 commits into from
Jul 5, 2023
Merged

Conversation

dottorblaster
Copy link
Contributor

Just externalized the checks selection into a component of its own. Stories available 🤘

@dottorblaster dottorblaster self-assigned this Jun 29, 2023
@dottorblaster dottorblaster force-pushed the host-checks-selection-frontend branch 2 times, most recently from 8a90f0d to e5b563a Compare June 30, 2023 10:50
@dottorblaster
Copy link
Contributor Author

@nelsonkopliku @EMaksy I will be away for a couple weeks, so I'll leave some notes on this to make the merge and the iteration (if any) easiest as it can be.

  • The logic that displays the selected checks has been simplified a bit
  • You can use the onClear handler to clear any redux state that holds any saga result. It gets called every time a check gets selected, and on mount
  • The component is completely drop-in: remaining stuff is using this to build the actual hosts check result page and dropping this into the old clusters checks selection page, making the same component declared locally into that directory (ClusterDetails) useless (and the directory itself slimmer).

Right now I don't have any other remark in my mind but feel free to ask!

@nelsonkopliku nelsonkopliku force-pushed the host-checks-selection-frontend branch 2 times, most recently from 6e02609 to 5c19e9f Compare July 4, 2023 07:44
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.

Fantastic job man!
Eager to see how it will be applied in the cluster and host views 🙂

@nelsonkopliku nelsonkopliku force-pushed the host-checks-selection-frontend branch 2 times, most recently from 3c2d3e9 to 27e715a Compare July 5, 2023 08:21
@nelsonkopliku
Copy link
Member

@arbulu89 addressed the feedbacks and took the opportunity to align naming from resource to target in 27e715a

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.

Thank you @nelsonkopliku
❤️
PD: Still added a comment XD, but no need of a new review, the code looks great

@nelsonkopliku nelsonkopliku force-pushed the host-checks-selection-frontend branch from 27e715a to 23572b4 Compare July 5, 2023 08:41
@nelsonkopliku nelsonkopliku merged commit 55baf80 into main Jul 5, 2023
@nelsonkopliku nelsonkopliku deleted the host-checks-selection-frontend branch July 5, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants