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

Pr/remove hooks modifying head #1004

Merged
merged 3 commits into from
Nov 9, 2022
Merged

Conversation

ArttuOll
Copy link
Contributor

@ArttuOll ArttuOll commented Nov 4, 2022

What is the problem?

In #386 it was decided that hooks modifying the contents of the <head> element should not be implemented and existing such hooks should be removed.

What changes does this PR make to fix the problem?

  • Delete useDocumentTitle (are there any other that should be removed?)
  • Update docs on useDocumentTitle and useFavicon with information about this decision and suggestion on using libraries such as react-helmet.
  • My editor was screaming about typos and spelling errors in the migration guide, so I fixed them.

Closes #386

…eFavicon

Hooks that modify contents of the head element will not be implemented, as discussed in #386.

closes #386
Removing this hook since it has been decided that hooks which modify contents of the head element
should not be included in this library.

BREAKING CHANGE: useDocumentTitle has been removed.
re #386
@ArttuOll ArttuOll requested a review from xobotyi November 4, 2022 13:23
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #1004 (75909a6) into master (7bcc385) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1004      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files          61       60       -1     
  Lines        1039     1025      -14     
  Branches      165      161       -4     
==========================================
- Hits         1035     1021      -14     
  Misses          2        2              
  Partials        2        2              
Impacted Files Coverage Δ
src/index.ts 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xobotyi
Copy link
Contributor

xobotyi commented Nov 4, 2022

LGTM, but! we have to fix useStorageValue first

@ArttuOll
Copy link
Contributor Author

ArttuOll commented Nov 9, 2022

One approving review and the CI is passing (coverage dropped merely because there's now fewer lines of code to test). Off to production we go!

@ArttuOll ArttuOll merged commit 12f723d into master Nov 9, 2022
@ArttuOll ArttuOll deleted the pr/remove-hooks-modifying-head branch November 9, 2022 05:41
github-actions bot pushed a commit that referenced this pull request Nov 9, 2022
# [18.0.0](v17.0.1...v18.0.0) (2022-11-09)

* Remove hooks that modify the head element (#1004) ([12f723d](12f723d)), closes [#1004](#1004) [#386](#386) [#386](#386) [#386](#386)

### BREAKING CHANGES

* useDocumentTitle has been removed.
@xobotyi
Copy link
Contributor

xobotyi commented Nov 9, 2022

🎉 This PR is included in version 18.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants