search

codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
24 stars 23 forks source link

Issue 651/reroute civic profile #656

Open andycwilliams opened 1 week ago

andycwilliams commented 1 week ago

This PR:

Resolves #651 and #634

Setting this as a draft for now.

1. Disables linking to incomplete Financial Info form 2. Reroutes clicking "Civic Profile" in NavBar from main Civic Profile page to the Basic Info form 3. Refactors Civic Profile navigation from simple menu to Material UI Tabs

Screenshots (if applicable):

2024-06-20 2024-06-20 (3) 2024-06-20 (2) 2024-06-20 (1)

Additional Context (optional):

These are pretty janky, imperfect solutions. I'm sure I can optimize it but in the interest of time, I'm at least starting out with this.

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

Look into code optimization and/or abstraction.

Ensure it's responsive on all screens, browsers, and other user conditions.

andycwilliams commented 5 days ago

Looking at the visual changes, I'd much prefer the vertical tabs as oppose to horizontal ones since we already have horizontal tabs in the main navbar.

The second thing that might be an issue if certainly the content jumping which looks a bit clunky.

Screen.Recording.2024-06-26.at.12.45.06.AM.mov Lastly, if we're cutting out Financial Information out for now and potentially adding it back in in the future, I would comment out the lines/tabs/components consisting of Financial Information like in the mobile view.

For mobile view, vertical looks nice but not for desktop, in my opinion. I can make it change based on screen size.

Do you mean completely commenting out all Financial Info code? Like everything FinancialInfo.jsx and its test?

leekahung commented 4 days ago

For mobile view, vertical looks nice but not for desktop, in my opinion. I can make it change based on screen size.

Do you mean completely commenting out all Financial Info code? Like everything FinancialInfo.jsx and its test?

Don't really like the Vertical tabs for mobile since there's only two and is not part of a drop down.

You mean horizontal tabs looks nicer in mobile than vertical since it takes up less space for the main content? I would prefer that. Other wise vertical tabs for desktop is good

And yes, just comment it the unused components

dbmzzo commented 4 days ago

The code looks good and works well for me. I tested on both Mac and iPhone, with a focus on narrower viewports. I find the tabs much better looking and functional than the menu.

The only issue I encountered was when I used autocomplete to fill in the form. Because of the presence of 'last' in the field names, autocomplete entered my last name in the ZIP and City fields. This can be easily fixed by adding the autoComplete prop to the inputs with the appropriate values. I can add inline comments for those potential changes.

andycwilliams commented 3 days ago

Alright, marking this as ready for review. The only lingering problem is the tests, but I plan on creating a new issue to address all of the failing ones anyway.

@leekahung It does look cleaner now. If and when the Financial form is ready, we may have to revisit tab arrangement but I don't think it's a limiting factor for now.

@dbmzzo Thanks for catching that. For some reason I can't quite replicate the issue but that's very likely because of the way I'm testing it. Also, since each field in the form has "last" in it I added an autoComplete value to each one rather than just those two.