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

upcoming: [DI-21322] - Retain Resource selections during expand / collapse of filter button #11068

Merged

Conversation

venkymano-akamai
Copy link
Contributor

@venkymano-akamai venkymano-akamai commented Oct 8, 2024

Description 📝

Bug fix for retaining the resource selections during expand / collapse of filter button

Changes 🔄

1.Bug fix for retaining the resource selections during expand / collapse of filter button

Target release date 🗓️

10-10-2024

Preview 📷

Include a screenshot or screen recording of the change

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2024-10-14 at 8 01 38 PM Screenshot 2024-10-14 at 8 00 41 PM

How to test 🧪

  1. Login as mock user
  2. Navigate to monitor tab
  3. Select a dashboard and in filters section select upto resources / cluster
  4. Collapse and expand the filter button, the selected resources should be retained

Reproduction steps

Clone the linode/manager/develop branch and do the following steps

  1. Login as mock user
  2. Navigate to monitor tab
  3. Select a dashboard and in filters section select upto resources / cluster
  4. Collapse and expand the filter button, the selected resources will not be retained

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Oct 10, 2024

Since this is a draft and you're looking for another alternative. I figured the issue might be with CloudPulseResourcesSelect.tsx itself. Here is my approach which seems to fix the issue and eliminate the way we're trying to micro-manage the renders:

export const CloudPulseResourcesSelect = ({
  defaultValue,
  disabled,
  handleResourcesSelection,
  placeholder = 'Select a Resource',
  region,
  resourceType,
  savePreferences,
  xFilter,
}: CloudPulseResourcesSelectProps) => {
  const { data: resources = [], isLoading } = useResourcesQuery(
    disabled !== undefined ? !disabled : Boolean(region && resourceType),
    resourceType,
    {},
    xFilter ?? { region }
  );

  const defaultResources = React.useMemo(() => {
    if (!defaultValue || !Array.isArray(defaultValue)) {
      return [];
    }
    const defaultIds = defaultValue.map(String);
    return resources.filter((resource) =>
      defaultIds.includes(String(resource.id))
    );
  }, [defaultValue, resources]);

  React.useEffect(() => {
    if (savePreferences && resources.length > 0) {
      handleResourcesSelection(defaultResources, true);
    }
  }, [savePreferences, resources, defaultResources, handleResourcesSelection]);

  return (
    <Autocomplete
      onChange={(_, resourceSelections) => {
        handleResourcesSelection(resourceSelections, savePreferences);
      }}
      textFieldProps={{
        InputProps: {
          sx: {
            maxHeight: '55px',
            overflow: 'auto',
            svg: { color: themes.light.color.grey3 },
          },
        },
        hideLabel: true,
      }}
      autoHighlight
      clearOnBlur
      data-testid="resource-select"
      defaultValue={defaultResources}
      disabled={disabled || isLoading}
      isOptionEqualToValue={(option, value) => option.id === value.id}
      label="Select a Resource"
      limitTags={2}
      multiple
      options={resources}
      placeholder={placeholder}
    />
  );
};

Since you know the overall feature better, let me know if that works.

@venkymano-akamai
Copy link
Contributor Author

venkymano-akamai commented Oct 12, 2024

Since this is a draft and you're looking for another alternative. I figured the issue might be with CloudPulseResourcesSelect.tsx itself. Here is my approach which seems to fix the issue and eliminate the way we're trying to micro-manage the renders:

export const CloudPulseResourcesSelect = ({
  defaultValue,
  disabled,
  handleResourcesSelection,
  placeholder = 'Select a Resource',
  region,
  resourceType,
  savePreferences,
  xFilter,
}: CloudPulseResourcesSelectProps) => {
  const { data: resources = [], isLoading } = useResourcesQuery(
    disabled !== undefined ? !disabled : Boolean(region && resourceType),
    resourceType,
    {},
    xFilter ?? { region }
  );

  const defaultResources = React.useMemo(() => {
    if (!defaultValue || !Array.isArray(defaultValue)) {
      return [];
    }
    const defaultIds = defaultValue.map(String);
    return resources.filter((resource) =>
      defaultIds.includes(String(resource.id))
    );
  }, [defaultValue, resources]);

  React.useEffect(() => {
    if (savePreferences && resources.length > 0) {
      handleResourcesSelection(defaultResources, true);
    }
  }, [savePreferences, resources, defaultResources, handleResourcesSelection]);

  return (
    <Autocomplete
      onChange={(_, resourceSelections) => {
        handleResourcesSelection(resourceSelections, savePreferences);
      }}
      textFieldProps={{
        InputProps: {
          sx: {
            maxHeight: '55px',
            overflow: 'auto',
            svg: { color: themes.light.color.grey3 },
          },
        },
        hideLabel: true,
      }}
      autoHighlight
      clearOnBlur
      data-testid="resource-select"
      defaultValue={defaultResources}
      disabled={disabled || isLoading}
      isOptionEqualToValue={(option, value) => option.id === value.id}
      label="Select a Resource"
      limitTags={2}
      multiple
      options={resources}
      placeholder={placeholder}
    />
  );
};

Since you know the overall feature better, let me know if that works.

Hi @jaalah-akamai Thanks for the suggestion , if you see in your suggested approach we observed couple of issues

  1. We don't have a state variable, we rely only on defaultValue.
  2. Here, preferences is not working properly, for example, if i select a resource, go to another page like linodes, longview and comeback to monitor page, since it is re-rendering, No values will be shown in the dropdown, also defaultValue is set only once during the initialization of the component.

@venkymano-akamai venkymano-akamai marked this pull request as ready for review October 14, 2024 14:29
@venkymano-akamai venkymano-akamai requested a review from a team as a code owner October 14, 2024 14:29
@venkymano-akamai venkymano-akamai requested review from mjac0bs and hkhalil-akamai and removed request for a team October 14, 2024 14:29
@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Oct 14, 2024
@bnussman-akamai
Copy link
Member

This is unrelated to this PR, but there is no spacing between this button and the Engine field. This should probably be fixed at some point. Having no spacing looks like a UI bug to me

Screen.Recording.2024-10-14.at.3.15.31.PM.mov

@kmuddapo
Copy link

This is unrelated to this PR, but there is no spacing between this button and the Engine field. This should probably be fixed at some point. Having no spacing looks like a UI bug to me

Screen.Recording.2024-10-14.at.3.15.31.PM.mov

Thanks!! for pointing it out, Its known thing and we are in progress of enabling the Titles/Labels for all these filters , so that will take care of enough space b/w filters and engine.

@venkymano-akamai
Copy link
Contributor Author

This is unrelated to this PR, but there is no spacing between this button and the Engine field. This should probably be fixed at some point. Having no spacing looks like a UI bug to me

@bnussman-akamai , this has been already taken care in the filter label task that is in progress and it is an upcoming PR, commit id in our local branch
ACLPManager@4a090df

@venkymano-akamai
Copy link
Contributor Author

@jaalah-akamai , we have enough approval here , Can we merge this?

@jaalah-akamai jaalah-akamai merged commit 8621de3 into linode:develop Oct 15, 2024
22 of 23 checks passed
Copy link

cypress bot commented Oct 15, 2024

Cloud Manager E2E    Run #6676

Run Properties:  status check failed Failed #6676  •  git commit 8621de36fe: upcoming: [DI-21322] - Retain Resource selections during expand / collapse of fi...
Project Cloud Manager E2E
Run status status check failed Failed #6676
Run duration 32m 29s
Commit git commit 8621de36fe: upcoming: [DI-21322] - Retain Resource selections during expand / collapse of fi...
Committer venkatmano-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 3
Tests that were flaky  Flaky 7
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 432

Tests for review

Failed  placementGroups/delete-placement-groups.spec.ts • 1 failed test

View Output Video

Test Artifacts
Placement Group deletion > can delete with Linodes assigned when unexpected error show up and retry Screenshots Video
Failed  oneClickApps/one-click-apps.spec.ts • 1 failed test

View Output Video

Test Artifacts
OneClick Apps (OCA) > Lists all the OneClick Apps Screenshots Video
Failed  helpAndSupport/open-support-ticket.spec.ts • 1 failed test

View Output Video

Test Artifacts
open support tickets > can create an SMTP support ticket Screenshots Video
Flakiness  linodes/resize-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
resize linode > resizes a linode by decreasing size Screenshots Video
Flakiness  linodes/clone-linode.spec.ts • 1 flaky test

View Output Video

Test Artifacts
clone linode > can clone a Linode from Linode details page Screenshots Video
Flakiness  objectStorageGen2/bucket-create-gen2.spec.ts • 3 flaky tests

View Output Video

Test Artifacts
Object Storage Gen2 create bucket tests > can create a bucket with E0 endpoint type Screenshots Video
Object Storage Gen2 create bucket tests > can create a bucket with E1 endpoint type Screenshots Video
Object Storage Gen2 create bucket tests > can create a bucket with E2 endpoint type Screenshots Video
Flakiness  placementGroups/delete-placement-groups.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Placement Group deletion > can unassign Linode when unexpected error show up and reopen the dialog Screenshots Video
Flakiness  kubernetes/smoke-lke-create.spec.ts • 1 flaky test

View Output Video

Test Artifacts
LKE Create Cluster > Simple Page Check Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Cloud Pulse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants