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: [M3-7694] - OBJ Multi Cluster Copy updates #10188

Merged
merged 10 commits into from
Feb 16, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Feb 13, 2024

Description πŸ“

Update final copy for OBJ Multi Cluster.

How to test πŸ§ͺ

Prerequisites

(How to setup test environment)

  • Turn on MSW
  • Turn on OBJ Multicluster

Verification steps

(How to verify changes)

  • Validate the copy updates mentioned in the ticket, referring to the copy changes from Figma linked within the ticket.
  • Ensure there no regression in Bucket, Access key Create / Edit flows when the flag and MSW is off.

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 February 13, 2024 15:22
@cpathipa cpathipa requested review from bnussman-akamai and jaalah-akamai and removed request for a team February 13, 2024 15:22
Copy link

github-actions bot commented Feb 13, 2024

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

@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Feb 14, 2024

I'm not sure this is related, but when going to http://localhost:3000/object-storage/access-keys with feature flag on and MSW off, the page breaks. Is the same happening for you?

@cpathipa
Copy link
Contributor Author

I'm not sure this is related, but when going to http://localhost:3000/object-storage/access-keys with feature flag on and MSW off, the page breaks. Is the same happening for you?

@jaalah-akamai This is handled in the PR #10189 to manage these errors gracefully when the feature flag is on and MSW is off. Once it is merged, we won't see the app crashing when MSW is off.

@mjac0bs mjac0bs self-requested a review February 15, 2024 16:15
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 Chandra! Confirmed copy changes matched the requested updates, but left a bit of feedback.

This is my first time looking at OBJ Multi-Cluster stuff in a while, so I also wanted to note an unrelated-to-this-PR UI feature that initially confused me:

Screenshot 2024-02-15 at 9 08 38 AM

After creating an access key, when viewing the modal, I thought the "Copy all" button applied to every field (copy the regions, access key, and secret key all at once) because I only had one region and "Copy all" was at the top. After seeing the HostNamesDrawer, I understand that it is intended to copy only the hostnames, of which there can be multiple. Not sure if this would be a common misconception, but might be worth discussing with UX, removing the "Copy all" in this modal, or changing it to "Copy" when there is just one region -- in a separate ticket.

Another minor note here is that the top and bottom padding on this field seem a little too wide.

cpathipa and others added 5 commits February 15, 2024 12:58
…776464.md

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

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@cpathipa
Copy link
Contributor Author

Thank you @mjac0bs Great feedback.

Copy All -

  • Hides Copy all when one hostname exists. (fad1190) - (UX approval)
  • Adjusted the padding for the field (3068f00).
    image

Error Message - UX is working on it, Will create a followup PR for better messaging.

@cpathipa cpathipa merged commit 8e5bb02 into linode:develop Feb 16, 2024
17 of 18 checks passed
@cpathipa cpathipa deleted the M3-7694-final-copy-update branch February 16, 2024 19:34
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.

None yet

4 participants