search

codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
30 stars 26 forks source link

Issue 358/render permissions forms #370

Closed brancwill closed 1 year ago

brancwill commented 1 year ago

name: Pull Request about: Create a pull request to merge your code into the PASS codebase title: ''358/render-permission-forms" labels: ''Enhancement" assignees: ''brancwill"

CONCISE description of PR (PR Title)

I changed the SetAclPermissionForm and SetAclPermsDocContainerForm into modals. The former is now opened by a button that I've attached to each file in the Document Table to open the modal with the File Name set to said file's name. The latter is opened by a button next to the Add Document button.

This PR:

1.
2. If needed, delete if not
3. If needed, delete if not

The files this PR effects:

Components

SetAclPermissionsModal.jsx (formerly SetAclPermissionsForm.jsx) SetAclPermsDocContainerModal.jsx (formerly SetAclPermsDocContainerModal.jsx) DocumentTable.jsx DocumentTableRow.jsx Profile.jsx index.js (in Forms and in Modals folder)

Tests

List file names here

Other Files

List file names here

Screenshots (if applicable):

Screenshot1 Screenshot2

Additional Context (optional):

Future Steps/PRs Needed to Finish This Work (optional):

@xscottxbrownx and I spoke about potentially condensing the table through either a menu containing the document actions (setting permissions, deletion, etc.) or through an accordion (@leekahung 's suggestion). If this is something we'd like to do, I can either add to this PR, or start another one.

Issues needing discussion/feedback (optional):

1. I'm pretty unfamiliar with MUI and JSDoc, so I copy and pasted a lot. With JSDoc in particular, I wasn't sure if I should add Modals to SetAclPermissionsModal and SetAclPermsDocContainerModal's @memberof or not. Feedback would be appreciated, I'm happy to tweak anything.

xscottxbrownx commented 1 year ago

@Jared-Krajewski I don't see any difference between this PR template and the Issue templates, but you can see that the extra stuff used to help GitHub was displayed...

Screenshot 2023-08-08 at 7 17 53 AM

If you have time, could you take a look and see what may be wrong?

brancwill commented 1 year ago

OK, I've made the changes

andycwilliams commented 1 year ago

I definitely like having the forms no longer just awkwardly placed under the document table.

I would suggest not using the settings icon for this since we already have it for the settings page in the navbar menu.

Some ideas for alternatives:

https://mui.com/material-ui/material-icons/

I'm not sure if the <Box> on line 99 of SetAclPermissionModal.jsx actually does anything. Might be able to remove it. Same with line 95 of SetAclPermsDocContainerModal.jsx.

Also, now that all of the forms are in modals and no longer directly on a webpage, we can maybe remove boxShadow: 2 on line 34 of FormSection.jsx. With this refactoring, it just creates an extra unnecessary line around the border for all modals.

brancwill commented 1 year ago

OK, I'll get all of these new changes incorporated soon. My weekend starts on Sunday, so I'll definitely have time then, if not sooner.

leekahung commented 1 year ago

Hey @brancwill, I've noticed that there's commits from other PRs mixed into this one. I'm thinking there could be a mix up when trying to resolve merge conflicts for this branch. Let me know if you need some help. I'll be available on Discord. 👍

brancwill commented 1 year ago

There was some confusion with merge conflicts, so I'm made a new branch and I'm making another PR