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

chore(style): remove jank from payment server load #2175

Merged
merged 1 commit into from Oct 9, 2019

Conversation

johngruen
Copy link
Contributor

This patch fixes a loading jank on the payment server with the following fixes:

  1. Brings minimal critical-path styles into a <style> to prevent the loading spinner from rendering incorrectly at the top of the document.
  2. Updates Index page html to better match that of the content server.

Copy link
Contributor

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

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

I'm seeing a perpetual loading spinner with this and redundant DOM elements. I think we're also trying to get away from inline styles for CSP, but I don't think we're there yet.

</head>

<body>
<div id="root">
Copy link
Contributor

Choose a reason for hiding this comment

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

id="root" is where the React app renders, once it's loaded. The contents here are just kind of pre-loading stuff until React comes in and replaces it all.

Moving the loading spinner above this in the DOM means the app is never visible. There's also a bunch of repeated elements here (static-footer, stage, etc) because React renders them in root.

Capture

@johngruen
Copy link
Contributor Author

Thanks Les...yeah, i'm going to revisit this without moving around stuff in the index so much...I'm not adding inline styles at all here. is internal CSS also verboten b/c of CSP?

@lmorchard
Copy link
Contributor

is internal CSS also verboten b/c of CSP?

Yeah, I think it's supposed to be, but we are kind of punting on it because Stripe components play with both if I recall

@johngruen
Copy link
Contributor Author

it looks like you can set a nonce in CSP rules and in a <style> tag that would make it acceptable...i have no idea how hard that would be...documentation is a little way down in this section of the CSP MDN page

@shane-tomlinson
Copy link
Contributor

Thanks Les...yeah, i'm going to revisit this without moving around stuff in the index so much...I'm not adding inline styles at all here. is internal CSS also verboten b/c of CSP

On the payments server unsafe-inline is used because the Stripe component uses inline style attributes and we can't control what they do. If that's the case, you should be fine to use a normal style tag here. Hopefully that makes using a nonce/sha moot:

it looks like you can set a nonce in CSP rules and in a <style> tag that would make it acceptable...i have no idea how hard that would be...documentation is a little way down in this section of the CSP MDN page

@johngruen
Copy link
Contributor Author

@lmorchard i kind of want to revisit this in 148... it looks like @shane-tomlinson says that using a style tag is fine.

@johngruen
Copy link
Contributor Author

actually I think i can do this by just getting rid of the pre-payload spinner

@@ -6,7 +6,7 @@ const spinnerImage =

export const LoadingSpinner = () => (
<div id="loading-spinner" data-testid="loading-spinner">
<img alt="Loading" src={spinnerImage} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kinda controversial i guess, but I don't think this image sticks around long enough for the alt text to have semantic importance (it could easily be a CSS background), and sometimes the word "Loading" flashes briefly on the screen on refresh

Copy link
Contributor

Choose a reason for hiding this comment

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

For this to be technically valid HTML, we should at least set it to an empty string (alt='').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kk, I can fix that

Copy link
Contributor

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

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

Looks fine to me, though others can chime in

@johngruen johngruen merged commit 7c9b9ae into master Oct 9, 2019
@shane-tomlinson shane-tomlinson deleted the dejank-payment-server-load branch October 14, 2019 11:56
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.

None yet

4 participants