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-7697] - Add scrolling for S3 hostnames in the Access Keys modal #10218

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Feb 22, 2024

Description πŸ“

Adds scroll bar to S3 endpoint hostnames in Access Keys Modal.

Before After
image image

How to test πŸ§ͺ

Prerequisites

(How to setup test environment)

  • Turn on OBJ Multi cluster flag
  • Turn on MSW

Verification steps

(How to verify changes)

  • Navigate to http://localhost:3000/object-storage/access-keys
  • Create Access keys with selecting all regions.
  • Verify Access keys and security key is visible without scrolling.
  • Turn off feature flag and MSW, ensure there is no regression in Creating Access key flow.

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 22, 2024 15:05
@cpathipa cpathipa requested review from dwiley-akamai and abailly-akamai and removed request for a team February 22, 2024 15:05
@cpathipa cpathipa self-assigned this Feb 22, 2024
@cpathipa cpathipa marked this pull request as draft February 22, 2024 15:05
@@ -59,6 +59,11 @@ export const SecretTokenDialog = (props: Props) => {

return (
<ConfirmationDialog
sx={() => ({
'.MuiPaper-root': {
overflow: 'hidden',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added to get rid of the unwanted scrollbar in the Modal.

@cpathipa cpathipa marked this pull request as ready for review February 22, 2024 15:09
Copy link

github-actions bot commented Feb 22, 2024

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

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.

Code review βœ…
Observed the intended scrolling βœ…
Confirmed no regressions with feature flag and MSW off βœ…

In dark mode, do we want the Hostnames box's border to match the color of the other fields?

Screenshot 2024-02-22 at 4 12 44 PM

@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Feb 23, 2024
Copy link
Contributor

@abailly-akamai abailly-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 fix works, but it reveals a UI issue (that we may have in a number of places)

if my OS has to only show scrollbars on scroll (system settings > Appearance):
Screenshot 2024-02-26 at 09 42 55

you will never know there's more items to scroll:
Screenshot 2024-02-26 at 09 41 19

The RemovableSelectionList has a drop shadow at the bottom to that effect which I think we want to implement on this component as well

@jdamore-linode
Copy link
Contributor

@cpathipa Seeing a small issue where the S3 endpoint field collapses under certain viewports (primarily mobile devices in landscape orientation):

Screenshot 2024-02-26 at 1 25 19 PM

@cpathipa
Copy link
Contributor Author

Great feedback @abailly-akamai and @jdamore-linode addressed PR feedback in the commit c684b0b

  • Added box shadow for the list.
  • viewport issue is fixed.

Copy link
Contributor

@abailly-akamai abailly-akamai 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 add the drop shadow @cpathipa, looks great πŸ’…

functionality and styling look good βœ…

I could see a case for adding a unit test for HostNamesList.tsx, feel free to add one

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Mar 5, 2024
@cpathipa cpathipa merged commit 48bf7e4 into linode:develop Mar 12, 2024
17 of 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! OBJ Multi-Cluster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants