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: Update action bar row layout in UserManagement page #9862

Merged
merged 19 commits into from
Oct 7, 2024

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Aug 23, 2024

Ticket

ET-299

Description

The user management page had a broken layout for the "add user" button on mobile view. This PR intends to fix that.

Test Plan

  • Log in as an admin
  • go to the "Admin" management page from the dropdown menu (that should take you to the user management page)
  • enable mobile view (from the terminal console, there is a "desktop" button on the top-left corner, that toggles the view mode)
  • the UI should have the "add user" button showing below the filters

Screenshot 2024-08-27 at 16 31 49

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.55%. Comparing base (ddca766) to head (78d0596).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9862      +/-   ##
==========================================
- Coverage   54.60%   50.55%   -4.05%     
==========================================
  Files        1259      949     -310     
  Lines      157329   128683   -28646     
  Branches     3619     3620       +1     
==========================================
- Hits        85903    65059   -20844     
+ Misses      71293    63491    -7802     
  Partials      133      133              
Flag Coverage Δ
harness ?
web 54.36% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
webui/react/src/pages/Admin/UserManagement.tsx 83.78% <100.00%> (-0.06%) ⬇️

... and 313 files with indirect coverage changes

Copy link

netlify bot commented Aug 23, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 78d0596
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66ff052bf9034d0008475ed9
😎 Deploy Preview https://deploy-preview-9862--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

can we fix the horizontal scroll issue?

Screen.Recording.2024-08-23.at.12.19.22.PM.mov

also can we confirm with a designer and a product manage if its a good approach to fix the ticket issue r? not sure if we wanna show add user button in new line.

@thiagodallacqua-hpe
Copy link
Contributor Author

@alexjdetermined could you take a look at this approach and give us your thought please 🙏

@thiagodallacqua-hpe
Copy link
Contributor Author

can we fix the horizontal scroll issue?

Screen.Recording.2024-08-23.at.12.19.22.PM.mov
also can we confirm with a designer and a product manage if its a good approach to fix the ticket issue r? not sure if we wanna show add user button in new line.

I believe that this is an animation from the browser, I've tested locally and got that animation too, but, no scroll is being kept...

I've already asked for validation from @alexjdetermined about the design changes

@keita-determined
Copy link
Contributor

I believe that this is an animation from the browser, I've tested locally and got that animation too, but, no scroll is being kept...

i dont think so. It doesn't bounce, i can just scroll horizontally.

@thiagodallacqua-hpe
Copy link
Contributor Author

I've updated the behavior for the mobile view, that's the best I could do before getting anything back from the design team... cc @alexjdetermined

Copy link
Contributor

@keita-determined keita-determined 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 is not fixed. could you please test it before requesting the review?

webui/react/src/pages/Admin/UserManagement.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

instead of randomly choosing the breakpoint, can we just build a good HTLM structure to support multiple viewports? The current implementation would cause the similar issues in the future if we add/remove buttons or other components.

@keita-determined keita-determined removed their assignment Sep 30, 2024
@thiagodallacqua-hpe
Copy link
Contributor Author

@alexjdetermined, could you please provide a mockup/wireframe for us here...

webui/react/src/hooks/useMobile.ts Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.tsx Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
webui/react/src/hooks/useMobile.ts Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@keita-determined keita-determined left a comment

Choose a reason for hiding this comment

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

mostly looks good to me.
only concern is that only 4 letters are visible in the search field and texts in the select boxes are truncated in a certain viewport. it wouldn't be visible if we add one more button.

image

webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
webui/react/src/pages/Admin/UserManagement.module.scss Outdated Show resolved Hide resolved
@thiagodallacqua-hpe
Copy link
Contributor Author

mostly looks good to me. only concern is that only 4 letters are visible in the search field and texts in the select boxes are truncated in a certain viewport. it wouldn't be visible if we add one more button.

image

I've changed the minmax for the selects to have a min of min-content, that should solve that issue, but, at the cost of reducing the width of the search input... but, I guess that we should get something from @alexjdetermined for that perhaps. cc @keita-determined

@ashtonG
Copy link
Contributor

ashtonG commented Oct 3, 2024

@keita-determined wouldn't a better solve for this be to have the menu switch to mobile mode at tablet sizes? looking at the rest of the table, is the content readable?

@keita-determined
Copy link
Contributor

@keita-determined wouldn't a better solve for this be to have the menu switch to mobile mode at tablet sizes? looking at the rest of the table, is the content readable?

Your suggestion is to use mobile view in the tablet size viewport, right? that sounds good to me

@thiagodallacqua-hpe thiagodallacqua-hpe merged commit 7e8dbac into main Oct 7, 2024
81 of 94 checks passed
@thiagodallacqua-hpe thiagodallacqua-hpe deleted the thiago/ET-299 branch October 7, 2024 17:14
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.

5 participants