search

codeAdrian / gatsby-omni-font-loader

Font loader optimized for maximum performance. Removes render-blocking font resources and loads them asynchronusly. Handle FOUT & FOUC with font loading status watcher. Supports both local-hosted fonts and web fonts.
MIT License
106 stars 7 forks source link

Change loaded font behavior #14

Closed fcisio closed 3 years ago

fcisio commented 3 years ago

Hi, I took liberty with two main things:

  1. The scope for Helmet #13
  2. The use of attribute instead of classname

    Helmet only prints classes to its target. This means that it can overwrite other classes that can be there. Using attributes makes sure that nothing is overwritten.

New CSS use:

html[wf-helvetica-now-display=loaded]: {}

Also, since it's already a breaking change, I felt that we might not need an option to choose the scope from body or html

fcisio commented 3 years ago

I backtracked a little bit... I think adding the classes without Helmet is a better solution, and that way it's not a breaking change.

codeAdrian commented 3 years ago

Hi @fcisio , thanks for your contribution. I have reviewed the PR and I have some comments. I had made some modifications and pushed the changes, hope you don't mind. This PR has provided a good idea and a solid foundation for the improvements that I've made.

I like the idea of replacing Helmet. However, I've had to refactor this to a custom hook since the component didn't return anything anymore and had to create a separate wrapper component that calls the hook. This structure makes more sense for this case.

I've refactored the className to classList since it handles adding classnames much better - avoids duplicated classNames, leading and trailing whitespaces, etc. Docs for the classList: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList

Check out the changes and let me know if they work for you. If everything is okay, I'll plan the release early next week after I give it one more look.

fcisio commented 3 years ago

I love the refactoring you did, looks awesome! Tested it on my end, and it works perfectly.

I pushed a new commit, simply adding a "wf-all--loaded" class to allow for simple CSS selectors when we don't need specificity (while we're at it).

Thanks!

codeAdrian commented 3 years ago

I don't think the final class name for all loaded fonts is needed since the individual class names can be concatenated in a CSS selector for the same effect. If I see a valid use-case, I'll consider implementing it in a separate PR and version release. At this point, I'm not sure if there is any.

If you revert that recent part of the PR today, I'll finalize it and merge it tomorrow.

I appreciate the contribution. Thank you!

fcisio commented 3 years ago

Hey @codeAdrian , Are we still looking into releasing this soon?

Thanks