Issue 456/home page structure enhancements #516
Conversation
xscottxbrownx
added
enhancement
xscottxbrownx
requested review from
ogorman89,
leekahung,
Raspber,
timbot1789,
milofultz,
andycwilliams and
Jared-Krajewski
There was a problem hiding this comment.
Just some thoughts. I possibly overlooked a few things in this initial look through.
The tests are failing here, but I'm assuming they're waiting to be updated once we settle on what direction we want to go?
Also, I believe we discuss (a little) about slightly changing the desktop layout by having the images and associated text alternate side-by-side. That could free up a bit of space and make it flow a little more nicely but wouldn't really work on the mobile version, which should probably just remain as is. Assuming we even want this change.
|
Latest attempt: 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 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 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 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 UX perspective |
Just tagged in milo to attempt to solve our idea for a better |
|
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 |
|
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? |
Ah, good catch! Yeah, it looks like there's an extra ref set within one of the |
| @@ -7,19 +7,19 @@ import createMatchMedia from '../../helpers/createMatchMedia'; | |||
| const MockKeyFeaturesDefault = () => <KeyFeatures description="Example Text" />; | |||
| const MockKeyFeaturesMobile = () => <KeyFeatures isReallySmallScreen description="Example Text" />; | |||
|
|
|||
| it('renders 67% width default', () => { | |||
| it('renders 85% width default', () => { | |||
There was a problem hiding this comment.
This is fine, but tests tied to specific CSS values like this probably won't be very resilient. I don't think we'd want this test to fail if someone decides that actually 84% is better for desktop than 85%. We may want to associate the test with more significant values/ranges. Here it looks like mobile always renders 100%, which seems like a pretty set value. So something like expect(descriptionStyles.width).not.toBe('100%') may be more reliable.
In general we may be able to make tests like 'It renders desktop display on desktop' which checks for a desktopLayout class, and then a 'It renders mobile display on mobile' which checks for a mobileLayout class. Then we can update the precise details of the class as we need without causing the tests to fail.
There was a problem hiding this comment.
Fully agreed and something discussed in depth in our discord DM.
Changed all to have a base value so .toBe(basevalue) & .not.toBe(basevalue)
This is SLIGHTLY more resilient, but not by much as when base value changes you have the same effect.
I think all CSS value based tests are no good. Maybe adjusting tests to be more resilient is a future issue for the code base?
There was a problem hiding this comment.
I think the bigger question is what exactly is this supposed to be testing? If we are testing whether or not our styles change at certain values, I think it would make more sense to create a function that tests whether this does what it's supposed to to set isReallySmallScreen in src/components/Messages/MessageButtonGroup.jsx:
const isReallySmallScreen = useMediaQuery('(max-width: 480px)');If we can test useMediaQuery, then we won't really need to use these tests, will we?
There was a problem hiding this comment.
I agree that they are very temperamental. Since they are directly coupled with hard coded values, it will either be annoying or ignored by future devs, which kind of invalidates the purpose of tests, which is an alarm for unexpected changes. All that being said, trying to ensure the DOM doesn't change based on what we perceive to be unrelated changes is useful, but that's a great case for snapshots, not behavior-driven tests.
If we can test behaviors for the user, like black-box testing, then that will be more resilient. And I think will also let us ask whether or not a test is useful, but that is only my opinion.
There was a problem hiding this comment.
I'm not sure I follow 100%, but I also believe that testing that react is doing react is silly and unnecessary. The maintainers of react and MUI are testing them, we don't need to as well. I don't think we need to test if useMediaQuery works either. Kind of the point of using a library.
I feel like the behavior (testing onclicks and such) should be done.
I'm not sold on checking css/styling or if mobile component is rendering in mobile screen or if desktop component is rendering in desktop screen. It will render whatever we tell it to render.
I'm not sure, I'm still studying.
test/pages/Home.test.jsx
Outdated
| describe('Home Page', () => { | ||
| afterEach(cleanup); | ||
| // if tests are failing check this value vs value in Home.jsx | ||
| const h1FontSize = '144px'; |
There was a problem hiding this comment.
This is the same thing. This is the current base value - so .toBe(basevalue) & other tests expect .not.toBe(basevalue).
There was a problem hiding this comment.
I'd rather it be something like this, so I can check the ordered of the rendered elements:
it('renders mobile', () => {
window.matchMedia = createMatchMedia(599);
render(<Home />);
const h1 = screen.getByTestId('testStack');
const { children } = h1;
expect(children[0].toEqual(<Typography />));
});then desktop would just be like expect(children[0].not.toEqual(<Typography />));
but can't seem to figure out syntax or where in docs to find much besides the matchers.
There was a problem hiding this comment.
Ok this seemingly works for the intention:
describe('Home Page', () => {
afterEach(cleanup);
it('renders with correct order of logoSection on mobile', () => {
window.matchMedia = createMatchMedia(599);
render(<Home />);
const h1 = screen.getByTestId('testStack');
const children = Array.from(h1.children);
expect(children.length).toBe(3);
expect(children[0].tagName.toLowerCase()).toBe('span');
expect(children[1].tagName.toLowerCase()).toBe('img');
expect(children[2].tagName.toLowerCase()).toBe('span');
});
it('rrenders with correct order of logoSection on desktop', () => {
window.matchMedia = createMatchMedia(1200);
render(<Home />);
const h1 = screen.getByTestId('testStack');
const children = Array.from(h1.children);
expect(children.length).toBe(2);
expect(children[0].tagName.toLowerCase()).toBe('span');
expect(children[1].tagName.toLowerCase()).toBe('span');
});
});There was a problem hiding this comment.
This to me still feels very fragile. We're testing the exact order of the DOM, when for the user, none of this matters. If the goal is to ensure that the DOM doesn't change, let's just rely on snapshots as that seems more accurate for the purpose. Otherwise, I think tests should be testing the behavior that the user will experience
There was a problem hiding this comment.
This to me still feels very fragile. We're testing the exact order of the DOM, when for the user, none of this matters. If the goal is to ensure that the DOM doesn't change, let's just rely on snapshots as that seems more accurate for the purpose. Otherwise, I think tests should be testing the behavior that the user will experience
But the goal for this was not to ensure the DOM doesn't change. Quite the contrary. In this particular component that is being tested, the DOM should be different for mobile and desktop. This tests just that, without the reliance on a css style check.
This is your h1 section that you figured out the nested syntax of.
…-enhancements--h1 Make `h1` hold full title while maintaining formatting
|
@milofultz feel like taking a look at these tests to see if anything sticks out to you as well? |
|
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 |
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:
Screen.Recording.2023-11-13.at.11.44.29.AM.mov
Future Steps/PRs Needed to Finish This Work (optional):
Home.jsxpage 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