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

Kcd/user management super admin view #343

Merged
merged 24 commits into from
Feb 10, 2025

Conversation

johanna-skylight
Copy link
Collaborator

@johanna-skylight johanna-skylight commented Feb 6, 2025

PULL REQUEST

Summary

This PR contains the new user management page. It includes the users, user groups and edit members/query sections. All elements are interactive but display static/dummy data. Integration with the server will be made in a second PR.

This PR also includes improvements to the Table component and the Header (dropdown menu) component.

Screenshot 2025-02-06 at 12 50 51 PM Screenshot 2025-02-06 at 12 50 21 PM

Related Issue

Partially implements #129 and fixes QUE-163

Additional Information

Checklist

  • Descriptive Pull Request title
  • Link to relevant issues
  • Provide necessary context for design reviewers
  • Update documentation

@johanna-skylight johanna-skylight marked this pull request as ready for review February 6, 2025 17:59
Copy link
Collaborator

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

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

left a couple of nitpicky comments, but good work overall!

For frontend PR's, we usually drop a slack message in the design channel for Michelle to do a pass once you're ready for her to look at it

Copy link

github-actions bot commented Feb 7, 2025

Coverage report for ./query-connector

St.
Category Percentage Covered / Total
🔴 Statements 53.8% 941/1749
🔴 Branches
53.48% (+0.17% 🔼)
292/546
🔴 Functions 52.47% 191/364
🔴 Lines 53.87% 883/1639

Test suite run success

72 tests passing in 9 suites.

Report generated by 🧪jest coverage report action from 84e7439

@johanna-skylight
Copy link
Collaborator Author

@fzhao99 I think I addressed all the comments. Thank you for catching all the broken tables 🙇‍♀️ they should be fixed now.

Copy link
Collaborator

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

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

great work on cleaning up the table code!

Copy link

linear bot commented Feb 7, 2025

@mikang
Copy link
Collaborator

mikang commented Feb 7, 2025

Hi @johanna-skylight , looks great! I have a few comments. Could we please make the following updates?

  • Have the dropdown menu (from the gear) be wider, so "User management" is on one line
  • Add toasts when something is changed. "User permissions successfully changed"
  • Change background of the search bar (in the side panel) to be white
  • Move all the content up, so there is a total of 48px of whitespace between the header (including the light blue info header) and the content. This would change the other screens like FHIR server configuration as well.
  • Is it possible to remove the "X" from the dropdown? Otherwise, some users may be "floating" and exist in the app without a user permission status.
image

I'm guessing that side panel search and connecting the table rows to the side panel data are not part of this PR - is that right? So, Jane Doe is not in the side panel

I'm not able to exit out of the side panel or change my cursor location when I have the side panel open. I can't navigate when I try tabbing from "2 queries" here (tabbing doesn't do anything). I'll do a broader accessibility sweep later - would you like to make this change now, or later?

image

@johanna-skylight
Copy link
Collaborator Author

@mikang Thanks for the review!

As part of this initial PR I have addressed below items:

  • Have the dropdown menu (from the gear) be wider, so "User management" is on one line. This is done.
  • Change background of the search bar (in the side panel) to be white. This has been fixed.
  • Move all the content up, so there is a total of 48px of whitespace between the header (including the light blue info header) and the content. This would change the other screens like FHIR server configuration as well. I have modified the upper margins in all the pages.
  • Is it possible to remove the "X" from the dropdown? Otherwise, some users may be "floating" and exist in the app without a user permission status. I have migrated the role dropdown to the same dropdown used in the query page which is a more simpler component that does not have the "X".

Items that I will not be addressing in this PR.

The toast will get implemented in a follow up PR that integrates the pages with the real data and will allow the user to do the role changes.

Drawer component accessibility issues. We should create a different ticket to address the component. Because a11y has not been a main focus in the application so far we will probably find that some of the existing components need enhancements. I think it would be a good idea to sync up with Janki to figure out how to treat that as a different workstream from the admin feature.

@mikang
Copy link
Collaborator

mikang commented Feb 10, 2025

All looks and sounds good, @johanna-skylight !! Thank you! Good to go from the design side 👍

@johanna-skylight johanna-skylight merged commit 8825092 into main Feb 10, 2025
12 checks passed
@johanna-skylight johanna-skylight deleted the kcd/user-management-super-admin-view branch February 10, 2025 20:37
katyasoup added a commit that referenced this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants