react-hookz
feat(useFavicon): port `useFavicon` from `react-use`
What new hook does?
Checklist
- [x] Have you read contribution guideline?
- [ ] Does the code have comments in hard-to-understand areas?
- [ ] Is there an existing issue for this PR?
- #33
- [x] Have the files been linted and formatted?
- [x] Have the docs been updated?
- [x] Have the tests been added to cover new hook?
- [x] Have you run the tests locally to confirm they pass?
@septs If the pull request is merged/accepted, could you please add the hacktoberfest-accepted label?
Codecov Report
Merging #386 (0371976) into master (532cc41) will not change coverage. The diff coverage is
100.00%.
@@ Coverage Diff @@
## master #386 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 61 62 +1
Lines 1065 1075 +10
Branches 184 186 +2
=========================================
+ Hits 1065 1075 +10
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/index.ts | 100.00% <100.00%> (ø) |
|
| src/useFavicon/useFavicon.ts | 100.00% <100.00%> (ø) |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
I noticed that this PR is quite old, so I figured it was time to give it some love.
I went over this PR and fixed things that needed to be fixed in order for this to be merged (added missing documentation, improved example, refactored code a little). Let's review this and get this one merged!
The biggest code change I made is changing the link rel from "shortcut icon" to just "icon". The "shortcut" link type is legacy and should not be used anymore, as explained in MDN (check the explanation for the "icon" link type).
Ah, about this one. i'm still making my mind, but tipping to decision not to implement hooks that modify <head> contents since it is more complicated that just throwing elements there. There is a better library called react-helmet and react-helmet-async doing this job pretty well, and, in most cases, even better and in more versatile than any hook possible.
Ah, about this one. i'm still making my mind, but tipping to decision not to implement hooks that modify contents since it is more complicated that just throwing elements there. There is a better library called react-helmet and react-helmet-async doing this job pretty well, and, in most cases, even better and in more versatile than any hook possible.
Just had a look at react-helmet-async, and fair enough, there seems to be more to the SSR side than I imagined.
Could you write your thoughts like this to the pull requests in the future? That way we would all be on the same page and unnecessary work could be avoided.
Should we close this one then and add a mention to the docs that this one will not be implemented?
but tipping to decision not to implement hooks that modify
contents
What are going to do with useDocumentTitle then? Deprecate it or keep it?
- this hook has to be rejected
- we have to remove hooks modifying
<head>contents (yes, it is a breaking change) - we have to add clarification on migration guide (docs) with suggestions to use
react-helmetorreact-helmet-async - we have to add clarification notes to issue with migration list
- this hook has to be rejected
- we have to remove hooks modifying
<head>contents (yes, it is a breaking change)- we have to add clarification on migration guide (docs) with suggestions to use
react-helmetorreact-helmet-async- we have to add clarification notes to issue with migration list
Sounds good!
:tada: This issue has been resolved in version 18.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket: