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

Uniform Target Checks Selection #1842

Merged
merged 6 commits into from
Sep 20, 2023
Merged

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Sep 19, 2023

Description

Refactors and uniforms Cluster and Host settings pages.

A new ChecksSelectionHeader component is added and both ClusterSettingsPage and HostSettingsPage make use of it and also declare page structure.

Got rid of HostChecksSelection.

A first step towards a better unification of settings page/checks selection for different targets.
Not yet totally satisfied, but still, a step forward.

Possible followup

We could have something like a ChecksSelectionContainer that looks like this

const [selection, setSelection] = useState([]);
useEffect(() => {
  setSelection(selectedChecks);
}, [selectedChecks]);

return (
  <>
    <ChecksSelectionHeader
      targetID={targetID}
      targetName={targetName}
      backTo={backTo}
      pageHeader={pageHeader}
      isSavingSelection={isSavingSelection}
      itCanStartExecution={canStartExecution(
        selectedChecks,
        isSavingSelection
      )}
      selection={selection}
      onSaveSelection={onSaveSelection}
      onStartExecution={onStartExecution}
    />
    {children}
    <ChecksSelection
      catalog={catalog}
      catalogError={catalogError}
      loading={catalogLoading}
      selectedChecks={selection}
      onUpdateCatalog={onUpdateCatalog}
      onChange={setSelection}
    />
  </>
);

and then in ClusterSettingsPage (and HostSettingsPage) have something like the following

return (
    <ChecksSelectionContainer
      targetID={clusterID}
      targetName={clusterName}
      backTo={
        <BackButton url={`/clusters/${clusterID}`}>
          Back to Cluster Details
        </BackButton>
      }
      pageHeader={
        <PageHeader>
          Cluster Settings for <span className="font-bold">{clusterName}</span>
        </PageHeader>
      }
      isSavingSelection={saving}
      itCanStartExecution={canStartExecution(selectedChecks, saving)}
      selectedChecks={selectedChecks}
      catalog={catalog}
      catalogError={catalogError}
      catalogLoading={catalogLoading}
      onUpdateCatalog={refreshCatalog}
      onSaveSelection={saveSelection}
      onStartExecution={requestChecksExecution}
    >
      {catalogWarningBanner[provider]}
      <ClusterInfoBox haScenario={type} provider={provider} />
    </ChecksSelectionContainer>
);

or even have one single TargetSettingsPage that accepts the target type and behaves accordingly.

deferring at the moment

How was this tested?

Relevant test adjsuted/added and stories updated.

@nelsonkopliku nelsonkopliku force-pushed the refactor-checks-execution branch from 79c65e3 to d9a4fff Compare September 19, 2023 11:24
@nelsonkopliku nelsonkopliku force-pushed the refactor-checks-execution branch from d9a4fff to cb6830d Compare September 19, 2023 13:40
@nelsonkopliku nelsonkopliku self-assigned this Sep 19, 2023
@nelsonkopliku nelsonkopliku added tech debt chore javascript Pull requests that update Javascript code labels Sep 19, 2023
@nelsonkopliku nelsonkopliku marked this pull request as ready for review September 19, 2023 14:13
Copy link
Contributor

@jamie-suse jamie-suse left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicks and we can go

Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@nelsonkopliku nelsonkopliku merged commit 9993ec2 into main Sep 20, 2023
@nelsonkopliku nelsonkopliku deleted the refactor-checks-execution branch September 20, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore javascript Pull requests that update Javascript code tech debt
Development

Successfully merging this pull request may close these issues.

3 participants