Issue 456/home page structure enhancements

xscottxbrownx commented 7 months ago

This PR:

Resolves #456

1. Added PASS logo and text to use as h1 - my understanding is we are switching to this logo. Need a higher quality version.
2. Refactored headers of home page to follow semantic outline.
3. Added <main> to layout - only thing marked as main semantically previously was this home logged out page, nothing else. 4. Added some conditional rendering to solve issue of adding empty elements to DOM.

Screenshots:

https://github.com/codeforpdx/PASS/assets/71395931/c47cc438-056a-49c7-bc3d-d7f5f0071013

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

Need to figure out what to do for proper headers in footer - wasn't sure because it depends on what page is being rendered currently (I think.)
Also believe the Home.jsx page can be refactored. Seems like some props are being passed to child components, that could just be initialized in those components themselves. Could also possibly be arrays of objects and .map()'ed - but not sure that would cut down code or improve readability until attempted. I do believe it would make for easier maintainability/changes in future.

Issues needing discussion/feedback (optional):

1. Styling feedback 2. Code review

xscottxbrownx commented 7 months ago

Latest attempt: https://github.com/codeforpdx/PASS/assets/71395931/1d89fa06-7f5e-46c6-a9ae-8fa78803f14c

Still have some code work to do for proper headings, then to tackle tests.

Let me know any feedback on this design.

andycwilliams commented 7 months ago

Latest attempt: https://github.com/codeforpdx/PASS/assets/71395931/1d89fa06-7f5e-46c6-a9ae-8fa78803f14c

Still have some code work to do for proper headings, then to tackle tests.

Let me know any feedback on this design.

The video isn't rendering but I downloaded it.

Overall I think it's getting better.

Line 70 I think could be an h4 on smaller viewports. On the desktop version it's fine.

Also, when I try out mobile I see this, where its not formatted as nicely as in the video.

I've tried reloading and resizing but it doesn't change. Not sure what's causing it.

Using our primary theme color instead of black would be nicer and more consistent in my opinion. I don't recall if we have a version of the PASS logo with that yet, though.

Also, we use a version of the logo with the PASS text within the image. So I'm not sure if we want to use that here as well. But it doesn't really seem necessary and would maybe limit our responsiveness a bit.

In case you weren't already aware, the Learn More button does not function correctly. Plus it overlaps with the other elements on certain viewports.

Honestly, at this point I'd just like to get UX's perspective. We may be winging it a bit too much.

xscottxbrownx commented 7 months ago

Latest attempt: https://github.com/codeforpdx/PASS/assets/71395931/1d89fa06-7f5e-46c6-a9ae-8fa78803f14c Still have some code work to do for proper headings, then to tackle tests. Let me know any feedback on this design.

The video isn't rendering but I downloaded it.

Overall I think it's getting better.

Line 70 I think could be an h4 on smaller viewports. On the desktop version it's fine.

Also, when I try out mobile I see this, where its not formatted as nicely as in the video.

I've tried reloading and resizing but it doesn't change. Not sure what's causing it.

Using our primary theme color instead of black would be nicer and more consistent in my opinion. I don't recall if we have a version of the PASS logo with that yet, though.

Also, we use a version of the logo with the PASS text within the image. So I'm not sure if we want to use that here as well. But it doesn't really seem necessary and would maybe limit our responsiveness a bit.

In case you weren't already aware, the Learn More button does not function correctly. Plus it overlaps with the other elements on certain viewports.

Honestly, at this point I'd just like to get UX's perspective. We may be winging it a bit too much.

video not rendering yeah it seems when github changed that you can't just click in the textbox and add an image/video, but now have to click the paperclip button to do so - they also limited video size. Any larger files (haven't figured out exact size yet) seem to make you download file instead of just play it.

Changed line 70 per your thoughts, looks better in my opinion.

Changed to green, but we don't have logo in that color yet that I could find.

I was told the logo that has the PASS text combined is not properly optimized and doesn't scale well, so I didn't attempt it.

Learn More Yeah, I haven't seemed to figure out anchor "scroll to" links in MUI yet. I'll dig into it - if you happen to know by chance, let me know.

UX perspective yeah pretty much most of the discussion now is not technically in the scope of this PR to fix the headings.

xscottxbrownx commented 7 months ago

Looks like this is failing a bunch of unit tests. If we still havent settled on a design for the front page, it may be worthwhile to loosen up these tests so they don't fail on areas we haven't finalized yet.

Just tagged in milo to attempt to solve our idea for a better h1 for this page, and I'm moving onto the tests. Honestly, looking into them I don't believe they are the best written and may need to write a new approach. But I'm just starting this now.

timbot1789 commented 7 months ago

I think you can remove the HomeSection unit tests. It seems to me that this is a section in too heavy development for those to be all that useful (and there's not too much interesting behavior on this page anyway).

The Home.jsx snapshot test will need to either be updated with npm run test -- --update-snapshot, or replaced with a similar render test that shows it's rendering the content.

xscottxbrownx commented 7 months ago

npm run test -- --update-snapshot

I don't know when this all happened, but seems like tests are failing all over on Development branch too. I can start working on those next. I'd like to learn more about testing.

So, my important question is - when do we want tests? When is it valuable to have them?

Like in the case of HomeSection: 1) switch it to see if the component returns anything 2) switch it to see if the component returns what we are currently expecting - maybe image and title text 3) delete the test

leekahung commented 7 months ago

npm run test -- --update-snapshot

I don't know when this all happened, but seems like tests are failing all over on Development branch too. I can start working on those next. I'd like to learn more about testing.

So, my important question is - when do we want tests? When is it valuable to have them?

Like in the case of HomeSection:

switch it to see if the component returns anything

switch it to see if the component returns what we are currently expecting - maybe image and title text

delete the test

We would like to check to see if an element within a component is rendered, that's one important test, especially if that element would be something users would interactive with like a button or a text field.

The next thing should be if there are any significant rendering changes between breakpoints (like if widths would change from 25% to 100%) think we should test their CSS properties as well. For these tests, think we could make it into a binary test, one where we use the base value for Desktop as a basis for those tests. So the tests would either .toBe(<what we expect the value to be on Desktop>) or .not.toBe(<what we expect the value to be on Desktop>). It'l be more flexible as well which simply shows that "rendering changes has occur".

Of course, if we change the base case (i.e. say Desktop for now), we'll have to update the test, but if we change the non-base case (i.e. mobile), the test should still run fine.

For snapshots, which has been in discussion for a while, I think we should outright replace with either #1 of what you've mentioned for the Home page. For #2, I think we could handle those when testing for smaller components like in the HomeSection component itself.

leekahung commented 7 months ago

I think you can remove the HomeSection unit tests. It seems to me that this is a section in too heavy development for those to be all that useful (and there's not too much interesting behavior on this page anyway).

The Home.jsx snapshot test will need to either be updated with npm run test -- --update-snapshot, or replaced with a similar render test that shows it's rendering the content.

Just to clarify, I'm in support of changing the snapshot the Home tests, but not removing the HomeSection tests (switch doesn't use snapshots), since they're mainly for detecting rendering changes between breakpoints, which can be quite significant from Desktop to Mobile. Think there's a better way of doing that rather than removing them.

Replacing the unit test from snapshots for Home.jsx would be good, I agree that there's simply too much in development for Home page at this moment. If we do end up removing one of the tests, think we can have Home.test.jsx removed instead of HomeSection.test.jsx since the unit tests for the contents within that page, like <HomeSection> and <KeyFeatures>, can be handled in the unit tests in those components.

timbot1789 commented 7 months ago

npm run test -- --update-snapshot

I don't know when this all happened, but seems like tests are failing all over on Development branch too. I can start working on those next. I'd like to learn more about testing.

So, my important question is - when do we want tests? When is it valuable to have them?

Like in the case of HomeSection:

switch it to see if the component returns anything

switch it to see if the component returns what we are currently expecting - maybe image and title text

delete the test

@xscottxbrownx Hmm I'm not getting those failures when I run npm run test on dev. Are all your dependencies up to date? You may want to run npm ci just to be safe.

I also see I gave you the wrong snapshot update command. It's actually npm run test -- --update. My bad. That shouldn't cause the test failures though. The bad argument should just cause the system to crash.

xscottxbrownx commented 7 months ago

npm run test -- --update-snapshot

I don't know when this all happened, but seems like tests are failing all over on Development branch too. I can start working on those next. I'd like to learn more about testing. So, my important question is - when do we want tests? When is it valuable to have them? Like in the case of HomeSection:

switch it to see if the component returns anything

switch it to see if the component returns what we are currently expecting - maybe image and title text

delete the test

@xscottxbrownx Hmm I'm not getting those failures when I run npm run test on dev. Are all your dependencies up to date? You may want to run npm ci just to be safe.

I also see I gave you the wrong snapshot update command. It's actually npm run test -- --update. My bad. That shouldn't cause the test failures though. The bad argument should just cause the system to crash.

yeah most of them are this: TypeError: Object.hasOwn is not a function

searching makes it seem that it's related to an outdate node version (older than 16.9), but that would be strange as didn't have this before with tests and my node. So, trying to update node now to new version - just having issues "switching" to the new version. Says version 20 is installed, but when running node -v it still lists 16.4

Also did npm ci with no change.

I also found the snapshot update option, but going to to cut to instead write an actual test without snapshot entirely.

timbot1789 commented 7 months ago

So, my important question is - when do we want tests? When is it valuable to have them?

@xscottxbrownx how do we address this without writing a book? I'll start by recommending a short video on the topic: https://www.youtube.com/watch?v=EZ05e7EMOLM

Unfortunately there's no easy answer to this, and on top of that you have a bit of an edge case with the home page. It doesn't really fit with the rest of the app.

IMO, the question I start with is "What do I want the thing to do? What's interesting about this thing?" The answer to that question tells me what I should test. So if I have a document uploader where I can name documents and set expiry dates, then I want the uploader to do 3 things:

Upload documents
name documents
set expiry dates on documents

Then I write 3 tests focused on those 3 things. If I'm interested in another thing, like it rejects documents larger than 1 gb. Then I add a 4th test for that. This process is often called "behavior-driven development."

For the homepage, it seems you're interested in one thing: it renders the expected home page. So you need at least 1 test.

What's not entirely clear to me is if there's anything else interesting on this page, or if you've really decided what you want this page to do yet. If there's something else about this page that you think interesting, like it switches layouts when it's on a mobile device, then you probably want a test for that as well. Generally, static content doesn't need significant testing, since it's not doing too many interesting things.

If there are sections where you're still undecided what they should do, you don't need tests for them, but you should make it obvious that they are placeholders.

timbot1789 commented 7 months ago

npm run test -- --update-snapshot

I don't know when this all happened, but seems like tests are failing all over on Development branch too. I can start working on those next. I'd like to learn more about testing. So, my important question is - when do we want tests? When is it valuable to have them? Like in the case of HomeSection:

switch it to see if the component returns anything

switch it to see if the component returns what we are currently expecting - maybe image and title text

delete the test

@xscottxbrownx Hmm I'm not getting those failures when I run npm run test on dev. Are all your dependencies up to date? You may want to run npm ci just to be safe. I also see I gave you the wrong snapshot update command. It's actually npm run test -- --update. My bad. That shouldn't cause the test failures though. The bad argument should just cause the system to crash.

yeah most of them are this: TypeError: Object.hasOwn is not a function

searching makes it seem that it's related to an outdate node version (older than 16.9), but that would be strange as didn't have this before with tests and my node. So, trying to update node now to new version - just having issues "switching" to the new version. Says version 20 is installed, but when running node -v it still lists 16.4

Also did npm ci with no change.

I also found the snapshot update option, but going to to cut to instead write an actual test without snapshot entirely.

Hmm I'm using node version 16.20. We currently only have version 16 pegged in the .nvmrc. We may need to update that if version 16.4 doesn't work.

If you have nvm installed, you can upgrade to 16.20 using that. nvm install 16.20 && nvm use 16.20

xscottxbrownx commented 7 months ago

@timbot1789 I installed nvm. And updated to 16.20, now tests working fine (except the 3 for home page and it's components, that I can now work on correctly.)

milofultz commented 7 months ago

@timbot1789 I installed nvm. And updated to 16.20, now tests working fine (except the 3 for home page and it's components, that I can now work on correctly.)

I updated the home section test because it was fairly straightforward from the code. Didn't want to update the snapshot test because it was a bit more involved 👍

xscottxbrownx commented 7 months ago

@timbot1789 I installed nvm. And updated to 16.20, now tests working fine (except the 3 for home page and it's components, that I can now work on correctly.)

I updated the home section test because it was fairly straightforward from the code. Didn't want to update the snapshot test because it was a bit more involved 👍

ok, thanks... I finished new Home.test.jsx last night - I'll double-check KeyFeatures.tst.jsx now.

xscottxbrownx commented 7 months ago

This is waiting on PR #531 to be merged in first. Then this could be merged, if approved.

andycwilliams commented 7 months ago

I'm getting this error. Not sure if it's really app breaking or not. If it doesn't interfere then this should be good. The home page looks much cleaner and more professional now.

And I'd like to have a primary color version of the logo but that can wait.

Also, just remembered that the "Request a demo" button goes directly to Hugh's email. Is that what we'll ultimately want?

leekahung commented 7 months ago

I'm getting this error. Not sure if it's really app breaking or not. If it doesn't interfere then this should be good. The home page looks much cleaner and more professional now.

And I'd like to have a primary color version of the logo but that can wait.

Ah, good catch! Yeah, it looks like there's an extra ref set within one of the <HomeSection> components (Line 111 in Home.jsx). That should be removed to get rid of the error. Only the parent <div> needs it.

xscottxbrownx commented 7 months ago

@milofultz feel like taking a look at these tests to see if anything sticks out to you as well?

milofultz commented 7 months ago

Honestly, an overhaul of making our test suites more focused, resilient, and effective to their intended purposes should probably be a different issue/PR, as this can become really side quest-y

codeforpdx / PASS