Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: split page layout component into two functions, simplifying testing #639

Merged
merged 43 commits into from
Nov 3, 2023

Conversation

hollandjg
Copy link
Contributor

@hollandjg hollandjg commented Oct 11, 2023

Prepare for handover to RENCI: split page layout components into two parts:

  • new components/PageLayout – this handles all of the styling and has a simple interface which doesn't "know" anything about the graphQL schema
  • layouts/Layout – this handles the gatsby-specific graphQL bits, and uses components/PageLayout for the actual rendering

Additional simplifications:

  • Move navbar config into the staticText block of the siteMetadata, to be consistent with the other staticText pieces

Drive-by-improvements, required for testing of the updated components:

  • Make components associated with Layout accept nulls and empty values for all of their arguments. This is required to allow the PageLayout to accept null arguments. This fixes the defaults site, which up until now had not reliably rendered anything in develop mode on some browsers (Safari, for instance).
  • Add more tests with empty and null values

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for gatsby-theme-project-portal-defaults ready!

Name Link
🔨 Latest commit 91e1d0a
🔍 Latest deploy log https://app.netlify.com/sites/gatsby-theme-project-portal-defaults/deploys/65451753f343280008f662c9
😎 Deploy Preview https://deploy-preview-639--gatsby-theme-project-portal-defaults.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for gatsby-theme-project-portal-ex-site ready!

Name Link
🔨 Latest commit 91e1d0a
🔍 Latest deploy log https://app.netlify.com/sites/gatsby-theme-project-portal-ex-site/deploys/65451753da476a00088d4c30
😎 Deploy Preview https://deploy-preview-639--gatsby-theme-project-portal-ex-site.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Oct 11, 2023

Deploy Preview for project-portal-storybook ready!

Name Link
🔨 Latest commit 91e1d0a
🔍 Latest deploy log https://app.netlify.com/sites/project-portal-storybook/deploys/65451753dd2e3c00081b7a9c
😎 Deploy Preview https://deploy-preview-639--project-portal-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hollandjg hollandjg changed the base branch from main to refactor/remove-project-portal-config-node-replace-sitemetadata October 11, 2023 14:15
@github-actions
Copy link

github-actions bot commented Oct 11, 2023

Note: This comment gets updated with every commit

Git SHA: 91e1d0afcb47ea25dd8969e5cc409212a790da92

Site: example-site

Pa11y test status: PASS

 {
    "total": 20,
    "passes": 20,
    "errors": 0,
    "results": {
        "http://localhost:9000/project/completed-project-nodate/": [],
        "http://localhost:9000/project/completed-project/": [],
        "http://localhost:9000/project/completed-project2/": [],
        "http://localhost:9000/project/ongoing-project-nodate/": [],
        "http://localhost:9000/project/ongoing-project/": [],
        "http://localhost:9000/project/ongoing-project2/": [],
        "http://localhost:9000/project/open-project-2/": [],
        "http://localhost:9000/project/open-project-nodate/": [],
        "http://localhost:9000/project/open-project/": [],
        "http://localhost:9000/project/open-project3/": [],
        "http://localhost:9000/project/open-project4/": [],
        "http://localhost:9000/project/open-project5/": [],
        "http://localhost:9000/project/open-project6/": [],
        "http://localhost:9000/completed/": [],
        "http://localhost:9000/": [],
        "http://localhost:9000/open/": [],
        "http://localhost:9000/ongoing/": [],
        "http://localhost:9000/about/": [],
        "http://localhost:9000/contact/": [],
        "http://localhost:9000/contact/thank-you/": []
    }
}
 

@jashlu
Copy link
Contributor

jashlu commented Oct 31, 2023

Note for Navbar: if logo doesn't exist, the site title is no longer aligned with the page links Do we need both Page Layout and Layout?

!! For the Page Layout, the small window still shows the logo, so I don't think this is working as expected

!! For the Project Team, the contact pictures aren't showing up in the XL window

!! For the Page Layout, the small window still shows the logo, so I don't think this is working as expected

Screenshot 2023-10-31 at 1 24 26 PM

We may want to revisit the styling for the SiteTitle, which is bundled with the expand navbar icon in the smaller screens. Also it may be hard to get it aligned with the page links, because the page links are slightly off-center, caused by the underline that could show up under each link

@hollandjg
Copy link
Contributor Author

Thanks for the feedback, @hetd54 and @jashlu ! I'll take another look at this.

@hollandjg
Copy link
Contributor Author

hollandjg commented Oct 31, 2023

Note for Navbar: if logo doesn't exist, the site title is no longer aligned with the page links
Screenshot 2023-10-31 at 15 42 00

  • Fix alignment if image is missing

@hollandjg
Copy link
Contributor Author

Do we need both Page Layout and Layout?

Good question. I'll see if I can convince you.

The layouts/Layout handles the graphql bit and converts the data { site { siteMetadata { staticText { navBar, bottomBanner, footer } } } } into { navBar, bottomBanner, footer }.

Having that simpler format means that testing the PageLayout is much simpler – the stories have really simple arguments in comparison to the layouts/Layout stories.

It also means that the components/PageLayout doesn't care at all about the graphql and the structure of the data, making it a bit simpler.

Is that enough to make it a worthwhile abstraction for you, or is it just a pain?

@hollandjg
Copy link
Contributor Author

!! For the Page Layout, the small window still shows the logo, so I don't think this is working as expected

I think this is where you mean:
Screenshot 2023-10-31 at 15 51 01

Is this the correct behavior? I think I'm missing something.

@hollandjg
Copy link
Contributor Author

hollandjg commented Oct 31, 2023

!! For the Project Team, the contact pictures aren't showing up in the XL window

Did you mean the XS? Because this is what I'm seeing:
Screenshot 2023-10-31 at 15 53 44

This looks like a feature unrelated to the changes I made. This applies to the Contact component for XS screen sizes.
Screenshot 2023-10-31 at 15 56 14

... and it arises because by default the images are hidden (hidden on 30)
Screenshot 2023-10-31 at 15 59 17

@hollandjg
Copy link
Contributor Author

We may want to revisit the styling for the SiteTitle, which is bundled with the expand navbar icon in the smaller screens. Also it may be hard to get it aligned with the page links, because the page links are slightly off-center, caused by the underline that could show up under each link

Part of this might be to merge the Navbar into a single component – makes it easier to format. I've done that here: #679

@hetd54
Copy link
Contributor

hetd54 commented Nov 2, 2023

Do we need both Page Layout and Layout?

Good question. I'll see if I can convince you.

The layouts/Layout handles the graphql bit and converts the data { site { siteMetadata { staticText { navBar, bottomBanner, footer } } } } into { navBar, bottomBanner, footer }.

Having that simpler format means that testing the PageLayout is much simpler – the stories have really simple arguments in comparison to the layouts/Layout stories.

It also means that the components/PageLayout doesn't care at all about the graphql and the structure of the data, making it a bit simpler.

Is that enough to make it a worthwhile abstraction for you, or is it just a pain?

Yep, I'm convinced!

@hetd54
Copy link
Contributor

hetd54 commented Nov 2, 2023

!! For the Project Team, the contact pictures aren't showing up in the XL window

Did you mean the XS? Because this is what I'm seeing:

Nope, for some reason it wasn't showing on XL screens, but it's fine now? Maybe storybook just had a hiccup.

@hetd54 hetd54 self-requested a review November 2, 2023 17:40
Copy link
Contributor

@hetd54 hetd54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful!! Thanks for fixing some old style things too <3

@hollandjg hollandjg added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit b49d447 Nov 3, 2023
15 checks passed
@hollandjg hollandjg deleted the refactor/simplify-page-layout-components branch November 3, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants