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

Misc updates and prep for portaltech. #89

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Misc updates and prep for portaltech. #89

merged 3 commits into from
Nov 2, 2020

Conversation

sherakama
Copy link
Member

READY FOR REVIEW

Summary

  • Misc updates and fixes
  • Readying for Portaltech onboarding

Review By (Date)

  • 11/4

Criticality

  • Medium

Review Tasks

  1. Review code
  2. See inline comments

Associated Issues and/or People

Resources

@@ -0,0 +1,37 @@
# Git normalization
Copy link
Member Author

Choose a reason for hiding this comment

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

Just another dot file :D

@@ -1 +1,2 @@
GATSBY_STORYBLOK_ACCESS_TOKEN={REPLACE THIS WITH THE ACCESS TOKEN FOR YOUR SPACE}
SECRET={ANY STRING YOU WANT}
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be needed for the auth stuff but can be used for other signing purposes and is already on netlify's environment vars.

Comment on lines +89 to +91
include_favicon: false,
crossOrigin: `use-credentials`,
icons: [],
Copy link
Member Author

Choose a reason for hiding this comment

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

We have external favicons and touch icons so we need to disable them in the manifest or we get 404 not found errors in the console.

gatsby-node.js Outdated
@@ -34,7 +34,7 @@ exports.createPages = ({ graphql, actions }) => {

const entries = result.data.allStoryblokEntry.edges
entries.forEach((entry, index) => {
let pagePath = entry.node.full_slug == 'home' ? '' : `${entry.node.full_slug}/`
let pagePath = entry.node.full_slug == 'home' ? '' : `${entry.node.full_slug}`
Copy link
Member Author

Choose a reason for hiding this comment

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

The ending slash was causing a number of // URLs from storyblok

also, not having the slash allows gatsby's routing to function better and navigate() works.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I added the slash because the Gatsby's check for current/active page only works if it has an ending slash 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Oh never mind - this isn't in the place where it matters so it should be fine

Copy link
Member Author

Choose a reason for hiding this comment

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

huh, ok, sounds like we need to do some trimming on the slug then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have doxn or a link to the code for the active page checks?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm.....so I guess the thing is if someone comes to a page via a link without the ending slash will not get the active class
https://deploy-preview-89--adapt-giving.netlify.app/how-to-make-a-gift/planned-giving/founding-grant-society

All the links within the site which uses the link component adds the trailing slash so if they navigate using links on the site, it's ok.

Copy link
Member Author

@sherakama sherakama Oct 30, 2020

Choose a reason for hiding this comment

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

Ah interesting. See: gatsbyjs/gatsby#7737

Looks like we have a bit of sanitization to do. FWIW, ending in a slash looks to be the way to go but you can specify the partiallySelected option to get at things that are double slashed or # anchored.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll revise.

@@ -2,7 +2,7 @@
# ##############################################################################
[build]
publish = "public"
functions = "netlify-functions"
functions = "lambda"
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to be a sort-of-a-standard from other projects

Comment on lines +18 to +23
Access-Control-Allow-Origin = "*"
X-Frame-Options = "SAMEORIGIN"
Content-Security-Policy = "form-action https:; frame-src *.stanford.edu *.storyblok.com *.kimbia.com *.stripe.com *.stripe.network"
Referrer-Policy = "origin-when-cross-origin"
Strict-Transport-Security = "max-age=2592000"
Feature-Policy = "vibrate 'none'; geolocation 'none'; midi 'none'; notifications 'none'; push 'none'; sync-xhr 'none'; microphone 'none'; camera 'none'; magnetometer 'none'; gyroscope 'none'; speaker 'none'; vibrate 'none'; fullscreen 'none'"
Copy link
Member Author

Choose a reason for hiding this comment

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

Some security hardening.

Comment on lines +65 to +68
[[redirects]]
from = "/api/*"
to = "/.netlify/functions/:splat"
status = 200
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows us to use the urls /api/something/something instead of netlify's /.netlify/functions/something/something

@sherakama
Copy link
Member Author

@katrialesser FYI

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Just that one thing about the active class check issue if someone access a page without adding a trailing slash at the end. Within site navigation is generally ok since our link component adds the trailing slash (in navigation, card links etc).

I'm off this afternoon. If you need this ready for Monday please feel free to merge this in. I've looked around in this branch's netlify build and things look normal to me (which is good 😄 ). We can figure out the active class thing later.

@sherakama
Copy link
Member Author

@yvonnetangsu revised slug/path sanitization.

Copy link
Contributor

@katrialesser katrialesser left a comment

Choose a reason for hiding this comment

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

Thanks @sherakama for the detailed comments about what the changes effectively do. Helps a lot to have that. Everything looks good to me.

@yvonnetangsu yvonnetangsu self-requested a review November 2, 2020 23:06
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Thank you for the slug revision - works well! GTG 🥳

@yvonnetangsu yvonnetangsu merged commit 5c8d979 into main Nov 2, 2020
@yvonnetangsu yvonnetangsu deleted the misc branch November 2, 2020 23:07
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