search

codeforpdx / PASS

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

[Enhancement] - Replace existing Contact List with MUI Data Grid #457

Closed leekahung closed 9 months ago

leekahung commented 9 months ago

This PR:

Updates existing table used for our Contact List with MUI Data Grid. The module for MUI Data Grid has been included to package.json. The column with originally with the link to the contact profile has been split up into the original text and a button icon in order to allow for filtering via MUI Data Grid. Other than that, the grid should function similarly to our previous table.

The unused ContactListTableRow.jsx is removed and a new component ContactProfileIcon.jsx is created for the React Router Link to the contact profile page.

The PR also includes a minor correction for contactProfile.profileImage in src/components/Profile/ProfileImageField.jsx. Before the profileImage would be incorrectly rendered because of the incorrect variable name, profileImg, which doesn't exist as part of contactProfile.

The column with pinning has been removed (UX team believes if there's already a filter + sorting capability with MUI Data Grid, this might not be necessary; it'll be better to have this moved as a type of status instead). The name from Person has been split up into "First Name" and "Last Name" for a better sorting experience.

The files this PR effects:

Components

Test Files

Other Files

Screenshots (if applicable):

https://github.com/codeforpdx/PASS/assets/14917816/76a2e574-7e2f-488c-8308-866ffff8d7ae

Future Considerations:

leekahung commented 9 months ago

If we could resolve the button nested in an a tag, that would be great. Thank you!

Thanks for the review @milofultz! I'll have the changes made in a bit.

andycwilliams commented 9 months ago

Is there an associated issue for this?

@milofultz This is valuable feedback, at least for my own understanding. More insight into accessibility is really helpful for the project.

leekahung commented 9 months ago

Is there an associated issue for this?

I was looking around the issues. There doesn't seem to be one for this or the ones that were there was closed.

timbot1789 commented 9 months ago

I think the data grid is a good enhancement over the current view. I especially like how we can easily add sorting to it (which may eliminate the need for pinning / favoriting).

How would you say it relates to this ticket, where I talk about converting the contacts list into a series of cards? I based that off of one of the figma designs UX submitted. image

leekahung commented 9 months ago

How would you say it relates to this ticket, where I talk about converting the contacts list into a series of cards? I based that off of one of the figma designs UX submitted.

I think we could certainly have this link this to that issue since it does update the UI.

If we want to, think we could also separate out different fields from contact (like first name, last name) in a follow-up PR to make better use of Data Grid's innate sorting capabilities instead of our existing sorting scheme.

andycwilliams commented 9 months ago

How would you say it relates to this ticket, where I talk about converting the contacts list into a series of cards? I based that off of one of the figma designs UX submitted.

I think we could certainly have this link this to that issue since it does update the UI.

If we want to, think we could also separate out different fields from contact (like first name, last name) in a follow-up PR to make better use of Data Grid's innate sorting capabilities instead of our existing sorting scheme.

When I last talked to UX about this, they said they were still working on the desktop version of these cards. So that combined with temporarily focusing on documentation recently, I haven't done a ton with #343 lately (and should really have moved that to the backlog).

I'm still willing to complete that, it would just mean waiting until this is merged before continuing.

Also, I would likely focus on #414 before moving onto #343.

timbot1789 commented 9 months ago

When I last talked to UX about this, they said they were still working on the desktop version of these cards. So that combined with temporarily focusing on documentation recently, I haven't done a ton with #343 lately (and should really have moved that to the backlog).

I'm still willing to complete that, it would just mean waiting until this is merged before continuing.

Also, I would likely focus on #414 before moving onto #343.

That sounds good to me. I personally like the card look more than the table. IMO it's more humanizing, and shows the data in a more familiar way. And I think it will work well in a desktop environment. The table view is good when you have several rows worth of data you need to look at all at once, but I don't know how often that will be the case here.

milofultz commented 9 months ago

Is there an associated issue for this?

I was looking around the issues. There doesn't seem to be one for this or the ones that were there was closed.

@andycwilliams @leekahung The issues I had when using VoiceOver on the data grid (which I'll admit isn't technically super proficient) were just basic navigation. In comparison to a data grid like this one from W3 which was super easy to navigate and interact with, the MUI one on their site was almost unusable.

I don't quite have to technical know how with grid yet to understand why this is the case, but I saw similar complaints in their backlog (https://github.com/mui/mui-x/issues/4749, https://github.com/mui/mui-x/issues/4658, https://github.com/mui/mui-x/issues/4282).

Again, don't know how much of this is them, us, or me, but I saw a little bit of signal confirming my suspicions in their backlog so thought I'd bring it up.

edit: oop misread the original post, but leaving this here for clarity anyway, sorry for the erroneous tag now :)

leekahung commented 9 months ago

Got everything updated from the suggestions. Let me know if there's still anything that needs to be reviewed for this PR.

leekahung commented 9 months ago

Hey @xscottxbrownx @andycwilliams. Think I've manage to touch on the things you guys mentioned. Let me know if there's still other things you guys need to review or it's good to go?

leekahung commented 9 months ago

Curious, is the grid also intended to be implemented with the documents table?

Yeah, I think the grid would work well for the document list as well. Think we could have an issue created for that.

xscottxbrownx commented 9 months ago

Issue with rendering size on skinny mobile devices (like Galaxy Fold and similar), but not too worried about for MVP.

leekahung commented 9 months ago

Issue with rendering size on skinny mobile devices (like Galaxy Fold and similar), but not too worried about for MVP.

Yeah, I believe those screen sizes are at around 280px width? At an earlier discussion, I believe we've talked about limiting the lower screen limit to about 300px. Think I could adjust it to make it fit the smaller size using vw for the width size on the box surrounding the grid.

https://github.com/codeforpdx/PASS/assets/14917816/7ccaa6fb-1468-4e81-b17b-7301bc07353b

leekahung commented 9 months ago

I'm seeing some large gaps between the navbar and table (see screenshot) when using larger viewports. Looks to be caused by justifyContent: 'center' on line 58 in Contacts.jsx

I could certainly pull the Data Grid back up closer the the Navbar without the justifyContent. I'll also make the grid use a set height to fill out the the spacing in the "Contacts" page. It would not affect the mobile view.

https://github.com/codeforpdx/PASS/assets/14917816/82e3a4d7-ebd7-4be2-ab09-c50660de02ca

I'd really like to hear from case workers about how small the screens they actually use are. It may not even be an issue to set a 300px limit. But it may take time to get feedback.

With regards to the mobile width limit, what we're making here should be quite flexible down to 280px and lower, so I don't think we need to worry about that point now. (As you can see below, the grid content stretches out to 380px, but the grid itself can remain small (around ~210px in the example clip) thanks to the horizontal scrolling implemented with the grid.)

https://github.com/codeforpdx/PASS/assets/14917816/f6f04cfc-fb9d-46b5-bb66-75efae38edea

leekahung commented 9 months ago

I believe the changes here are now adequate?