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: [M3-6903] - Replace Select with Autocomplete in: volumes #10437

Merged
merged 9 commits into from
May 30, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented May 3, 2024

Description 📝

This PR eliminates redundant logic and reuses ConfigSelect in AttachVolumeDrawer.
Replaced Select with Autocomplete in ConfigSelect component.
Reused ConfigSelect instead of Select in AttachVolumeDrawer component.

Target release date 🗓️

6/10

How to test 🧪

Verification steps

(How to verify changes)

  • Navigate to http://localhost:3000/volumes and click on attach menu option.
  • Verify there is no regression in volume attach / detaching behaviors.
  • Verify there is no regression in Volume create flow.
  • Navigate to http://localhost:3000/linodes/< Linode_id>/storage - Verify there is no regression in creating / attaching / detaching volumes.

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

@cpathipa cpathipa requested a review from a team as a code owner May 3, 2024 02:42
@cpathipa cpathipa requested review from bnussman-akamai and hkhalil-akamai and removed request for a team May 3, 2024 02:42
@cpathipa cpathipa marked this pull request as draft May 3, 2024 02:43
Copy link

github-actions bot commented May 3, 2024

Coverage Report:
Base Coverage: 82.29%
Current Coverage: 82.29%

@cpathipa cpathipa self-assigned this May 3, 2024
@cpathipa cpathipa marked this pull request as ready for review May 8, 2024 15:22
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Looks good! Confirmed that Volume create/attach/detach flows are working as expected, and messed around with the autocomplete for a while and didn't see anything out of the ordinary. Thanks @cpathipa!

@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label May 8, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Now that we use an Autocomplete, we don't need to keep the options in the shape of { label: string, value: number }. Can we update remove configList and pass configs directly to the Autocomplete?

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Creating, attaching, and detaching function as expected ✅

If you open the Attach drawer but don't select a linode for a few seconds, this error pops up:

Screenshot 2024-05-08 at 5 03 42 PM

There's also more space between the fields on this branch compared to prod.

@hkhalil-akamai
Copy link
Contributor

If you open the Attach drawer but don't select a linode for a few seconds, this error pops up:

I'm also observing this. Also, the spacing with the error appears off, there seems to be some extra padding on the left side.

@cpathipa
Copy link
Contributor Author

Thank you for the feedback, I will look into that..

@cpathipa
Copy link
Contributor Author

Creating, attaching, and detaching function as expected ✅

If you open the Attach drawer but don't select a linode for a few seconds, this error pops up:

Screenshot 2024-05-08 at 5 03 42 PM

There's also more space between the fields on this branch compared to prod.

@dwiley-akamai @hana-linode This issue was fixed in the commit b8dbb10

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

The issue I noted previously has been fixed ✅

Another issue I noticed (that also happens in prod) is when you try to detach a volume from the Linode Storage tab, the table data doesn't get updated accordingly without a refresh.

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Seeing all regressions fixed! Thanks @cpathipa!

@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels May 30, 2024
@cpathipa cpathipa merged commit 5561201 into linode:develop May 30, 2024
18 checks passed
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! Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants