[Enhancement] - Issue-682/create documents route - #683
Conversation
…e contact documents list; Created new route for new Documents page Co-authored-by: Jared Krajewski <krajewski.jared@gmail.com>
leekahung
added
the
refactor/optimization
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Seeing some oddities with Shared Profile with resizing and overlapping elements.
Small screen (this happens when I resize the screen when on a shared profile. But doesn't seem to if I'm already on a mobile view on Contacts and then go to a shared profile):
Medium screen (placeholder documents table also cuts off at the bottom):
Large screen:
Also maybe the placeholder text for shared profiles should be something more like "No documents have been shared with you. Any that are shared will appear here".
Other thoughts, likely for another PR. I think we should style the Documents and Contacts mobile versions more consistently. The Share/Add buttons are not spaced from the screen sides the same way. It also looks like the text in Documents is bolded.
Similarly, for the desktop versions, we should probably center the Add Contact button since we're centering the buttons for Documents now.
|
With the main problem with the issue resolved, think we can open up a new PR to handle all the styling corrections. |
yeah, I think that sounds good. Is that good with you @andycwilliams ? |
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
This strange responsivity is still occurring. I'm not comfortable show this to a prospective investor. We want to put our best foot forward.
There's other tidying that could be done but I would really like this to be addressed.
| import { DocumentTable } from '@components/Documents'; | ||
| import { Container } from '@mui/system'; | ||
|
|
||
| const Documents = () => { |
There was a problem hiding this comment.
Given it's a page and not exactly a component, didn't think it'll be necessary to write one, but I can include a short documentation
| import { DocumentListContext } from '@contexts'; | ||
| import { truncateText } from '@utils'; | ||
| import { DocumentTable } from '@components/Documents'; | ||
| import { Container } from '@mui/system'; |
There was a problem hiding this comment.
Is there a reason Container is imported from '@mui/system' rather than '@mui/material' like it is elsewhere?
There was a problem hiding this comment.
No reason, it looks like the import path auto completed to the wrong path
I'll change it back
Which is why I recommend fixing this in a quick follow-up with this PR being the main push for component changes. Currently working on those styling fixes in a local branch, as stated in an earlier comment about styling corrections. I've just pushed it up as a PR (see PR #684) |
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Think it's good practice to address things like the responsivity issue. But since another PR is up now I'm more okay with approving this.
Don't think it's bad practice to have dedicated PRs for specific issues at hand, one with the main feature update, and then a follow-up to address bugs, unless the initial PR features breaks functionality. This is merging to the Development branch and not the Main/Master branch just yet where it's used for deployment. |
3 participants
This PR:
Resolves #682 and migrates Documents list from Profile to new Documents page and re-purpose Documents list for contacts using state management.
Screenshots (if applicable):
Screen.Recording.2024-10-09.at.5.09.14.PM.mov