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

refactor(core): Convert Cluster filters to react #8272

Merged
merged 6 commits into from
May 13, 2020

Conversation

caseyhebebrand
Copy link
Contributor

Create ClusterFilter react component. Remove unused angular code, and ensure backward compatibility.

Note: FilterCheckbox component already existed in two places. This PR just pulls it out into its own file so the load balancers, clusters, and functions can use it in their filter sections.

@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

Copy link
Contributor

@christopherthielen christopherthielen left a comment

Choose a reason for hiding this comment

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

👏 Really good work! The new ClusterFilters component is really easy to follow.

});

useOnStateChanged(() => {
ClusterState.filterModel.asFilterModel.activate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be checking if the new state is actually the Clusters State?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. The previous version does not do that, but adding that check makes sense from an efficiency standpoint.

trueKeyObjectToLabelFilters(sortFilter.labels),
);

const labelsMap = buildLabelsMap(serverGroupData);
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to memoize this for when serverGroupData is large.

Suggested change
const labelsMap = buildLabelsMap(serverGroupData);
const labelsMap = useMemo(() => buildLabelsMap(serverGroupData), [serverGroupData]);

@christopherthielen
Copy link
Contributor

Please fix the commit message Make input controlled before rebasing and merging

@caseyhebebrand caseyhebebrand merged commit f3cc20f into spinnaker:master May 13, 2020
@caseyhebebrand caseyhebebrand deleted the react-cluster-filters branch May 13, 2020 23:45
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.

3 participants