Enhancement add responsive mobile view for documents table#606 - #665
Conversation
…mponents/cards. Currently no document utilites reimplemented; i.e Share, Replace, Remove, and View.
…ated eventHandlers for cards to use DocumentTable logic. Delete doesn't work, but preview does.
…w ' ' -> '%20' convention. All buttons functional.
…lement mobile switch to document cards and responsiveness.
…ponents (DocumentsDesktop, DocumentsMobile). Remove redundant handler redefinitions.
…w-for-documents-table#606
…w-for-documents-table#606
…ments components. Adopt Contacts stylization methodology for desktop documents.
… mobile documents. Actions follow same dropdown effect with appropriate icons and remain functional.
andycwilliams
added
enhancement
|
Getting some errors as soon as I log in. I can't access any of the pages 
|
…nature of hooks, using UUID instead of useId() to avoid changes in hook order.
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Quick preliminary review. Will be more thorough later.
The app starts up correctly now. Good work so far.
- Looks like the header names in the column titles were changed or removed.
-
The mobile card lists the file name but nothing else. I'd like all the other information listed as well. Can just list out the rest, like the type, description, etc.?
-
Also, could we use the same icon for "Preview File"? Right now it uses a different one on the mobile version
|
Hi Josh. Any updates? |
…unique ids as keys to action items.
…ith established convention
…o use Flexbox. Move styles into reuseable constants.
There was a problem hiding this comment.
Doesn't appear to impact functionality, but there are some React warnings related to the mobile Drawers that does not exist for the desktop Modals.
Another bug is related to the text fields themselves where the previous value remains in the Drawer text fields despite being edited to be an empty string:
Seems to leak over to the Modal as well in the desktop version:
I'm guessing the state did not reset after the edit submission?
Not related to this PR specifically, so I'll move this into an issue instead. |
|
Planning on going over this issue tonight and working out a timetable for MVP |
|
For the description, let's set a max character limit to 140 characters with client-side validation. From Solid's perspective, I don't think there's a limit, but we could have CSS cap the visual length. That should be enough give a decent enough description of what the file is about without needing to preview the document and not making the card too larget. |
|
Had a check in with Josh and Andrew tonight. MVP is very close now. Styling should be pretty much done and most of what remains is just writing tests. When you have time, would appreciate a review. It's super possible to get MVP done and dusted by end of the month. Also I saw you approved the Oliver's PR. Haven't had a chance to look at that myself but if you think it's good to go then merge it. |
leekahung
left a comment
leekahung
left a comment
There was a problem hiding this comment.
Nothing too major, but there are a few things to note, especially with the file name for the document cards. See feedback.
…till retaining distinctive document data formatting
…to the right to match Contacts.
… to be more distinct from data.
…ling into constants like in other components.
|
I believe all aesthetic and functional concerns expressed have been addressed. Working on getting the tests out. |
leekahung
left a comment
leekahung
left a comment
There was a problem hiding this comment.
Think I can approve after the unit tests pass
…gin. Extract sx data to constants (where apt).
…w-for-documents-table#606
…w-for-documents-table#606
|
|
…ble#606' of https://github.com/codeforpdx/PASS into enhancement-add-responsive-mobile-view-for-documents-table#606
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Haven't had time to review every bit yet but looking pretty good so far. Couple of requests for the time being
| const dataStyling = { | ||
| ...rowStyling, | ||
| color: 'text.secondary', | ||
| fontSize: '.875rem', |
There was a problem hiding this comment.
Is there a particular reason rem is used here when px is used almost every other time?
There was a problem hiding this comment.
Not really, I think at the time it was just a little easier for me to scale font sizes to each other that way. I can change it to px for consistency.
| expect(screen.getByTestId('loading-animation')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('renders DocumentsDesktop for larger screens', () => { |
There was a problem hiding this comment.
Aren't these two tests very similar to the final two?
There was a problem hiding this comment.
I was wondering where our conventional balance between redundancy and thoroughness lies. I may have opted for the latter in more cases than necessary. I'll look into filtering moot redundancy out.
There was a problem hiding this comment.
Yes I see it now, they were actually nearly identical. Just something I'd overlooked. I removed the redundant final two in question.
… cards. Remove redundant test cases. Align test names with convention. Align CSS units with convention.
…le-view-for-documents-table#606
4 participants
This PR:
Resolves #606
1. Documents have a proper mobile view
2. Is consistent with Contacts style/methodology
Additionally:
Screenshots (if applicable):
Future Steps/PRs Needed to Finish This Work (optional):