codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
28 stars 25 forks source link

Issue 358 - Putting SetAclPermissions into Modal #388

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: ''issue-358/render-permission-forms" labels: ''Enhancement" assignees: ''brancwill"


CONCISE description of PR (PR Title)

Same PR as #370 (See reason for closing in original PR) 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

src/components/Form/SetAclPermissionsForm.jsx (now SetAclPermissionsModal.jsx) src/components/Form/SetAclPermsDocContainerForm.jsx (now SetAclPermissionsModal.jsx) src/components/Form/FormSection.jsx src/components/Documents/DocumentTable.jsx src/components/Documents/DocumentTableRow.jsx

Tests

List file names here

Other Files

src/pages/Profile.jsx src/components/Modals/index.js src/components/Form/index.js src/typedefs.js


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.

brancwill commented 1 year ago

@leekahung The changes have been made 👍

xscottxbrownx commented 1 year ago

Hey @brancwill,

just checking in with you and seeing how things are going?

Any questions, or clarification needed?

brancwill commented 1 year ago

Hey @xscottxbrownx , sorry for my absence on this. I made a lot of the changes y'all suggested.

Screenshot (14)

I tried including the document name into the title. I don't think it's too wide, but I would like the text to be centered. Not sure how you do that with MUI though.

timbot1789 commented 1 year ago

I tried including the document name into the title. I don't think it's too wide, but I would like the text to be centered. Not sure how you do that with MUI though.

@brancwill I think you can use the align property on the Typography element for that.

https://mui.com/material-ui/api/typography/#props

andycwilliams commented 1 year ago

Quick request: Could we add a character limit to displaying the file name? Currently it stretches out the modal and adds a scroll bar at the bottom. I think we can cut it off after a point to simplify usage. Should be easy enough to add.

2023-08-29 (16)

Here's an example from UploadDocumentModal.jsx starting on line 163:

<FormHelperText
  sx={{
    width: '200px',
    whiteSpace: 'nowrap',
    overflow: 'hidden',
    textOverflow: 'ellipsis'
  }}
>
  File to upload: {file ? file.name : 'No file selected'}
</FormHelperText>
leekahung commented 1 year ago

Hey @brancwill was wondering about the progress of this PR. If there's any problem like needing some help with the merge conflict, let us know. Thanks!

leekahung commented 1 year ago

The merge came out pretty good, good work @xscottxbrownx! Still a bit iffy about switching the shared icon with an outlined version, but that's just cosmetic, the PR is still good to go.

How about @timbot1789 and @andycwilliams?

xscottxbrownx commented 1 year ago

The merge came out pretty good, good work @xscottxbrownx! Still a bit iffy about switching the shared icon with an outlined version, but that's just cosmetic, the PR is still good to go.

We can change it, the others were all outlined versions - so just made it consistent. But I'm not married to it, nor really care either way.

xscottxbrownx commented 1 year ago

Switched back to share icon, instead of outlined version.

leekahung commented 1 year ago

Think this is mostly ready. Going to merge this into Development. Any further issues related to this PR could be addressed in future PRs.