Skip to content

Change loaded font behavior #14

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

Merged
merged 7 commits into from
Jan 19, 2021
Merged

Conversation

fcisio
Copy link

@fcisio fcisio commented Jan 12, 2021

Hi, I took liberty with two main things:

  1. The scope for Helmet Add classes to HTML instead of the BODY #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
Copy link
Author

fcisio commented Jan 13, 2021

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.

  • Implemented the scope option

@codeAdrian
Copy link
Owner

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
Copy link
Author

fcisio commented Jan 13, 2021

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
Copy link
Owner

codeAdrian commented Jan 13, 2021

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!

Francis Champagne and others added 2 commits January 13, 2021 12:56
@fcisio
Copy link
Author

fcisio commented Jan 16, 2021

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

Thanks

@codeAdrian codeAdrian merged commit ba8cf83 into codeAdrian:main Jan 19, 2021
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.

2 participants