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

fix: [M3-7864] - Make the ACL (Object storage) select field carat (^) symbol consist with other select fields in the CM. #10286

Merged
merged 14 commits into from
Apr 1, 2024

Conversation

cpathipa
Copy link
Contributor

Before After
image image

How to test πŸ§ͺ

Verification steps

(How to verify changes)

  • Navigate to Object storage bucket details Access tab and verify updating ACL.
  • Navigate to Object details drawer and verify updating ACL.
  • Verify AccessSelect references and ensure there is no regression.

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

… symbol consist with other select fields in the CM.
@cpathipa cpathipa requested a review from a team as a code owner March 14, 2024 16:51
@cpathipa cpathipa requested review from bnussman-akamai and hkhalil-akamai and removed request for a team March 14, 2024 16:51
… symbol consist with other select fields in the CM.
@cpathipa cpathipa self-assigned this Mar 14, 2024
@cpathipa cpathipa added the Bug Fixes for regressions or bugs label Mar 14, 2024
Copy link
Contributor

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

I'm not seeing the default value populate. I couldn't figure out why at first glance.

Develop This PR
Screenshot 2024-03-14 at 2 09 02β€―PM Screenshot 2024-03-14 at 2 06 59β€―PM

@cpathipa
Copy link
Contributor Author

[882d90e](/linode/manager/pull/10286/commits/882d90edafb8a55cc3beb5ae7fa76c61e9ea6906)

@bnussman-akamai Addressed this issue in882d90e

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.

Nice clean up -- one fewer legacy select component! πŸŽ‰

@bnussman-akamai
Copy link
Contributor

Unit tests need to be fixed!

Copy link

github-actions bot commented Mar 25, 2024

Coverage Report: βœ…
Base Coverage: 81.72%
Current Coverage: 81.72%

@cpathipa cpathipa requested a review from a team as a code owner March 26, 2024 14:26
@cpathipa cpathipa requested review from jdamore-linode and bnussman-akamai and removed request for a team March 26, 2024 14:26
@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Mar 29, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

It looks like we've lost some loading state, in defaulting to Private. It's impossible to tell in dark mode because of the color of our disabled field, but visible in light.

This Branch:

ThisBranchNoLoadingState.mov

Prod:

ProdWithLoadingState.mov

cpathipa and others added 4 commits April 1, 2024 09:06
@cpathipa
Copy link
Contributor Author

cpathipa commented Apr 1, 2024

@mjac0bs Enabled the loading state of for ACL select field. Looks like MUI Autocomplete is not showing the loading state when filtered values are provided. 1d2bbdc

ACL_LoadingState.mov

@cpathipa cpathipa requested a review from mjac0bs April 1, 2024 14:57
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for the fix - confirmed the loading state is back for ACL via the Access tab and in the edit drawer. Tests are all passing.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Apr 1, 2024
@cpathipa cpathipa merged commit 1f3440f into linode:develop Apr 1, 2024
18 checks passed
@cpathipa cpathipa deleted the M3-7864 branch April 1, 2024 16:48
bnussman-akamai pushed a commit to bnussman-akamai/manager that referenced this pull request Apr 4, 2024
… symbol consist with other select fields in the CM. (linode#10286)

* fix: [M3-7864] - Make the ACL (Object storage) select field carat (^) symbol consist with other select fields in the CM.

* Added changeset: Make the ACL (Object storage) select field carat (^) symbol consist with other select fields in the CM.

* Update selected value according to Autocomplete

* Update broken unit test

* Update AccessSelect.test.tsx

* Add e2e tests to view and update Bucket access (ACL)

* Update tests for AccessSelect refactor

* Remove mocking

* Update packages/manager/cypress/e2e/core/objectStorage/object-storage.e2e.spec.ts

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>

* Update packages/manager/.changeset/pr-10286-fixed-1710435081738.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>

* Show loading state for ACL select field.

---------

Co-authored-by: Joe D'Amore <jdamore@linode.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
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! Bug Fixes for regressions or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants