feat(useFavicon): port `useFavicon` from `react-use` #386

alexnaiman · 2021-10-09T21:11:31Z

What new hook does?

Checklist

Have you read contribution guideline?
Does the code have comments in hard-to-understand areas?
Is there an existing issue for this PR?
- Port remaining hooks from react-use #33
Have the files been linted and formatted?
Have the docs been updated?
Have the tests been added to cover new hook?
Have you run the tests locally to confirm they pass?

src/useFavicon/useFavicon.ts

Change from `document.getElementsByTagName('head')` to `document.head.`

alexnaiman · 2021-10-10T10:55:25Z

@septs If the pull request is merged/accepted, could you please add the hacktoberfest-accepted label?

septs · 2021-10-10T18:24:37Z

Sorry, i am not a maintainer

# Conflicts: # src/index.ts

…ve example This commit adds remaining things that are required for adding this hook to the library. Missing documentation is added, example is improved, ESLint warnings fixed and import style fixed. re react-hookz#33

codecov · 2022-10-30T15:33:00Z

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%> (ø)`

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

ArttuOll · 2022-10-30T15:33:43Z

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).

xobotyi · 2022-10-30T17:37:24Z

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.

ArttuOll · 2022-10-30T19:55:48Z

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?

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

ArttuOll · 2022-11-04T12:59:45Z

but tipping to decision not to implement hooks that modify contents

What are going to do with useDocumentTitle then? Deprecate it or keep it?

xobotyi · 2022-11-04T13:03:18Z

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-helmet or react-helmet-async
we have to add clarification notes to issue with migration list

ArttuOll · 2022-11-04T13:06:23Z

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-helmet or react-helmet-async

we have to add clarification notes to issue with migration list

Sounds good!

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

* docs(migrating-from-react-use): Add mention about not implementing useFavicon Hooks that modify contents of the head element will not be implemented, as discussed in #386. closes #386 * docs(migrating-from-react-use): Fix typos and spellling errors * feat(useDocumentTitle): Remove useDocumentTitle 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

# [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 · 2022-11-09T05:46:41Z

🎉 This issue has been resolved in version 18.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat(useFavicon): port useFavicon from react-use

901f75d

septs reviewed Oct 9, 2021

View reviewed changes

src/useFavicon/useFavicon.ts Outdated Show resolved Hide resolved

refactor(useFavicon): Change after code review

cf88bc4

Change from `document.getElementsByTagName('head')` to `document.head.`

xobotyi force-pushed the master branch from 555a4f2 to 2cb82a3 Compare December 23, 2021 12:46

xobotyi force-pushed the master branch from 29d7f8e to d86f5b8 Compare August 8, 2022 00:05

ArttuOll added 2 commits October 30, 2022 16:14

Merge branch 'master' into pr/useFavicon

81f128b

# Conflicts: # src/index.ts

ArttuOll requested review from xobotyi and JoeDuncko October 30, 2022 15:31

ArttuOll added a commit that referenced this pull request Nov 4, 2022

docs(migrating-from-react-use): Add mention about not implementing us…

77b6b27

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

ArttuOll mentioned this pull request Nov 4, 2022

Pr/remove hooks modifying head #1004

Merged

ArttuOll closed this in #1004 Nov 9, 2022

xobotyi added the released label Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(useFavicon): port `useFavicon` from `react-use` #386

feat(useFavicon): port `useFavicon` from `react-use` #386

alexnaiman commented Oct 9, 2021

alexnaiman commented Oct 10, 2021

septs commented Oct 10, 2021

codecov bot commented Oct 30, 2022

ArttuOll commented Oct 30, 2022 •

edited

Loading

xobotyi commented Oct 30, 2022 •

edited

Loading

ArttuOll commented Oct 30, 2022

ArttuOll commented Nov 4, 2022

xobotyi commented Nov 4, 2022

ArttuOll commented Nov 4, 2022

xobotyi commented Nov 9, 2022

feat(useFavicon): port useFavicon from react-use #386

feat(useFavicon): port useFavicon from react-use #386

Conversation

alexnaiman commented Oct 9, 2021

What new hook does?

Checklist

alexnaiman commented Oct 10, 2021

septs commented Oct 10, 2021

codecov bot commented Oct 30, 2022

Codecov Report

ArttuOll commented Oct 30, 2022 • edited Loading

xobotyi commented Oct 30, 2022 • edited Loading

ArttuOll commented Oct 30, 2022

ArttuOll commented Nov 4, 2022

xobotyi commented Nov 4, 2022

ArttuOll commented Nov 4, 2022

xobotyi commented Nov 9, 2022

feat(useFavicon): port `useFavicon` from `react-use` #386

feat(useFavicon): port `useFavicon` from `react-use` #386

ArttuOll commented Oct 30, 2022 •

edited

Loading

xobotyi commented Oct 30, 2022 •

edited

Loading