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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-plugin-typography): sort typography style tag first to avoid overwriting issues #12295

Merged
merged 2 commits into from
Mar 5, 2019
Merged

fix(gatsby-plugin-typography): sort typography style tag first to avoid overwriting issues #12295

merged 2 commits into from
Mar 5, 2019

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Mar 5, 2019 •
edited
Loading

Fixes #12295
Fixes #6302
Fixes #9911
Fixes #1994

@KyleAMathews
fix(gatsby-plugin-typography): Ensure that Typography.js styles are a…
d841750 
…t the top of the <head> so they don't overwrite other styles
@KyleAMathews KyleAMathews mentioned this pull request Mar 5, 2019
Closed
@DSchau
Copy link
Contributor

DSchau commented Mar 5, 2019

Looks good to me--added some unit tests to validate behavior.

Merging and publishing!

screen shot 2019-03-04 at 7 13 35 pm

DSchau
DSchau approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

👍

@DSchau DSchau changed the title fix(gatsby-plugin-typography): Ensure that Typography.js styles are at the top of the <head> so they don't overwrite other styles fix(gatsby-plugin-typography): sort typography style tag first to avoid overwriting issues Mar 5, 2019
@DSchau DSchau merged commit 5caf95e into master Mar 5, 2019
@DSchau DSchau deleted the typography-order branch March 5, 2019 01:16
@DSchau
Copy link
Contributor

DSchau commented Mar 5, 2019

Successfully published gatsby-plugin-typography@2.2.8

@KyleAMathews
Copy link
Contributor Author

🎉

@octalmage
Copy link
Contributor

woohoo, glad to see this make it in! Thanks!

@jbolda
Copy link
Contributor

jbolda commented Mar 6, 2019

I am still seeing some things inlined differently. I can reproduce it in the examples on the theme I have thrown together. Particularly noticeable with the Links being light green vs blue on the /articles/ page.

@krizzu
Copy link

krizzu commented Mar 7, 2019 •
edited
Loading

I've come across the same issue today.
My problem was that my CSS files were added to Head before typography.js injection, hence few styles were overridden.

So I've fixed it by overriding injectStyles method from typography instance.

// override method before exporting typography
typography.injectStyles = function() {
  if (typeof document !== 'undefined') {
    // Replace existing
    const styleNode = document.getElementById('typography.js');
    if (styleNode !== null) {
      styleNode.innerHTML = this.toString();
    } else {
      const node = document.createElement('style');
      node.id = 'typography.js';

      // note: use typography context 
      node.innerHTML = typography.toString();

	  // note here: I use prepend, not appendChild
      document.head.prepend(node);
    }
  }
};

@KyleAMathews
Copy link
Contributor Author

@jbolda @krizzu I assume this is development? @DSchau has a PR open for the same change as @krizzu at KyleAMathews/typography.js#201

@jbolda
Copy link
Contributor

jbolda commented Mar 7, 2019

@KyleAMathews yep, thanks for the heads up!

@DSchau
Copy link
Contributor

DSchau commented Mar 7, 2019

@jbolda any interest in validating the changes in that PR with your repo?

You can just build out and copy over files, or use npm|yarn link with the PR Kyle linked above.

Would help us assert that it's actually the fix--although it seems pretty obvious that is the fix!

@KyleAMathews
Copy link
Contributor Author

@DSchau's PR with the injectStyles fix is merged and published. Upgrade to typography@0.16.19

@robert-claypool
Copy link

Now that <style id="typography.js"> is forced to the top of <head>, I don't know how to include my own reset CSS file, or any other file that I want evaluated before typography.js CSS.

Ejecting default-html.js to src/html.js, and adding <link rel="stylesheet" href="reset.css" /> is now useless since there is no way to keep that reset tag above typography.js CSS. A reset should be the baseline on which typography.js and other custom CSS override.

@robert-claypool
Copy link

This PR also pushes <meta charset="utf-8"> down, but UTF-8 must be completely within the first 1024 bytes of the document, see https://github.com/h5bp/html5-boilerplate/blob/7.0.1/dist/doc/html.md#the-order-of-the-title-and-meta-tags

@KyleAMathews
Copy link
Contributor Author

@robert-claypool the expectation is that you'd use the normalize.css file Typography.js includes. Is there something special about your reset? Perhaps we could add support for custom resets to Typography.js. The assumption is that Typography.js is your reset — so you wouldn't need anything more than that.

The second issue should definitely be fixed. Would you like to give it a go at fixing it? You'd need to edit the sort so that it puts meta charset above typography.js

gatsby/packages/gatsby-plugin-typography/src/gatsby-ssr.js

Lines 26 to 32 in 632f035

headComponents.sort((x, y) => {
if (x && x.key === `TypographyStyle`) {
return -1
} else if (y && y.key === `TypographyStyle`) {
return 1
}
return 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DSchau DSchau DSchau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KyleAMathews @DSchau @octalmage @jbolda @krizzu @robert-claypool