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

Cluster settings page refactor #1659

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

dottorblaster
Copy link
Contributor

Description

Now the cluster settings page uses the new ChecksSelection visual component.

How was this tested?

Automated tests migrated

@dottorblaster dottorblaster self-assigned this Jul 21, 2023
@dottorblaster dottorblaster changed the base branch from main to host-checks July 24, 2023 08:12
@dottorblaster dottorblaster force-pushed the cluster-settings-page-aligned branch 3 times, most recently from 82e7002 to 9bc7e87 Compare July 25, 2023 13:12
@dottorblaster dottorblaster marked this pull request as ready for review July 25, 2023 13:14
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.

Nice job! Just a couple of tiny things and it's good to go for me.

I guess having Save Check Selection and Start Execution buttons on the top right as per figma design will follow up.

} = useSelector(getCatalog());

if (!cluster) {
return <div>Loading...</div>;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency we could use the LoadingBox component here

Suggested change
return <div>Loading...</div>;
return <LoadingBox text="Loading..." />;

return <div>Loading...</div>;
}

const { provider, type, selected_checks: selectedChecks } = cluster;
Copy link
Member

Choose a reason for hiding this comment

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

We could destructure clusterName here as well instead of using getClusterName function imported from ClusterLink on line 12. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that

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.

LGTM

@dottorblaster dottorblaster merged commit e076dfb into host-checks Jul 26, 2023
18 checks passed
@dottorblaster dottorblaster deleted the cluster-settings-page-aligned branch July 26, 2023 14:43
nelsonkopliku pushed a commit that referenced this pull request Aug 3, 2023
* Remove old ChecksSelection UI

* Rewrite ClusterSettingsPage

* Wire up ClusterSettingsPage to the router
dottorblaster added a commit that referenced this pull request Aug 4, 2023
* Add navigation to host check selection (#1658)

* Align cluster/host naming related to checks selection and actions

* Add Checks related Call to actions in host detail

* Host detail - Move exporters mapping to components in a separate variable

* Cluster settings page refactor (#1659)

* Remove old ChecksSelection UI

* Rewrite ClusterSettingsPage

* Wire up ClusterSettingsPage to the router

* Make checks selection great again (#1682)

* Add a new selector for cluster checks

* Update ChecksSelection component to new UX

* Update ClusterSettingsPage to new UX

* Update Host Settings to new UX

* Add a disabled prop to Button component

* Align the behavior of the save checks button

* fixup! Align the behavior of the save checks button

* Improved tooltip (#1674)

* Install react-tooltip

* Add new Tooltipo component

* Update Tooltip stories

* Add a global css object for tests

* Replace legacy tooltip with new one

* Use spans as tooltip anchor wrapper

* Replace react-tooltip with rc-tooltip

Co-authored-by: Alessio Biancalana <alessio.biancalana@suse.com>

* Switch Tooltip implementation to rc-tooltip

Co-authored-by: Alessio Biancalana <alessio.biancalana@suse.com>

---------

Co-authored-by: Alessio Biancalana <alessio.biancalana@suse.com>

---------

Co-authored-by: Alessio Biancalana <alessio.biancalana@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants