Skip to content

Migrate detect os hook - #5322

Merged
ovflowd merged 18 commits into
nodejs:major/website-redesignfrom
FHachez:migrate_dected_os_hook
Apr 24, 2023
Merged

Migrate detect os hook#5322
ovflowd merged 18 commits into
nodejs:major/website-redesignfrom
FHachez:migrate_dected_os_hook

Conversation

@FHachez

@FHachezFHachez commented Apr 21, 2023•
edited by ovflowd
Loading

Copy link
Copy Markdown
Contributor

Hey here is a first PR on the detect OS hook.

As discussed in the issue, I switch to relying on the userAgent on top of the appVersion, because the appVersion is deprecated.

  • I'm adding some tests.

@vercel

vercelBot commented Apr 21, 2023•
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

NameStatusPreviewCommentsUpdated (UTC)
nodejs-org✅ Ready (Inspect)Visit Preview💬 Add feedbackApr 24, 2023 1:45pm
nodejs-org-stories✅ Ready (Inspect)Visit PreviewApr 24, 2023 1:45pm

FHachez
FHachez commented Apr 21, 2023
View reviewed changes
Comment threadhooks/useDetectOs.ts Outdated
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 21, 2023 16:56 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 21, 2023 16:56 Inactive
ovflowd
ovflowd approved these changes Apr 21, 2023
View reviewed changes

@ovflowdovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Thank you for your first contribution!!

I've left a few comments 👀

(Don't forget to add tests before we get this merged) (Covering the util and the Hook separately)

Comment threadhooks/useDetectOs.ts Outdated
Comment threadutil/detectOS.ts Outdated
Comment threadutil/detectOS.ts Outdated
Comment threadutil/detectOS.ts Outdated
Comment threadutil/downloadUrlByOS.ts Outdated
Comment threadutil/downloadUrlByOS.ts Outdated
Comment threadutil/downloadUrlByOS.ts Outdated
@ovflowd
ovflowd self-requested a review April 21, 2023 17:03
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 21, 2023 17:42 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 21, 2023 17:43 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 21, 2023 17:45 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 21, 2023 17:45 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 21, 2023 18:07 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 21, 2023 18:08 Inactive
@FHachez

FHachez commented Apr 21, 2023

Copy link
Copy Markdown
ContributorAuthor

@ovflowd thanks for the review, I've tackled the comments.

@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 21, 2023 18:09 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 21, 2023 18:10 Inactive
shanpriyan
shanpriyan approved these changes Apr 21, 2023
View reviewed changes

@shanpriyanshanpriyan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR 🚀

@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 22, 2023 03:39 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 22, 2023 03:40 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 22, 2023 07:58 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 22, 2023 08:00 Inactive
ovflowd
ovflowd reviewed Apr 22, 2023
View reviewed changes

@ovflowdovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff! Those are really good contributions! Thank you for your effort <3

I left a few comments and suggestions 🙇

Comment threadhooks/__tests__/useDetectOS.test.ts
Comment threadhooks/__tests__/useDetectOS.test.ts
Comment threadhooks/__tests__/useDetectOS.test.ts
Comment threadutil/detectOS.ts Outdated
Comment threadutil/detectOS.ts Outdated
Comment threadutil/downloadUrlByOS.ts Outdated
Comment threadutil/downloadUrlByOS.ts Outdated
Comment threadhooks/useDetectOs.ts Outdated
Comment threadutil/detectOS.ts Outdated
Comment threadutil/detectOS.ts Outdated
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 22, 2023 10:28 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 22, 2023 10:29 Inactive
ovflowd
ovflowd reviewed Apr 22, 2023
View reviewed changes
Comment threadutil/downloadUrlByOS.ts Outdated
FHachez
FHachez commented Apr 22, 2023
View reviewed changes
Comment threadutil/downloadUrlByOS.ts Outdated
ovflowd
ovflowd reviewed Apr 22, 2023
View reviewed changes
Comment threadutil/detectOS.ts Outdated
ovflowd
ovflowd reviewed Apr 22, 2023
View reviewed changes
Comment threadutil/detectOS.ts Outdated
ovflowd
ovflowd reviewed Apr 22, 2023
View reviewed changes
Comment threadutil/detectOS.ts Outdated
ovflowd
ovflowd approved these changes Apr 22, 2023
View reviewed changes

@ovflowdovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff! Thank you for the changes. I left a few final comments, and we're ready to go!!

:shipit:

@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 22, 2023 10:36 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 22, 2023 10:37 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 22, 2023 10:46 Inactive
ovflowd
ovflowd approved these changes Apr 22, 2023
View reviewed changes

@ovflowdovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's get this merged! Congrats for your first contribution :)

@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 22, 2023 10:48 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org-stories April 24, 2023 13:44 Inactive
@vercel
vercelBot temporarily deployed to Preview – nodejs-org April 24, 2023 13:45 Inactive
@ovflowd
ovflowd merged commit f49c166 into nodejs:major/website-redesignApr 24, 2023
@ovflowdovflowd mentioned this pull request Apr 24, 2023
Closed
ovflowd pushed a commit that referenced this pull request May 3, 2023
@FHachezWai.Tung
@aymen94Claudio Wunder
Migrate detect os hook (#5322)
d910567
Co-authored-by: Wai.Tung <maledong_public@foxmail.com>
Co-authored-by: Aymen Naghmouchi <aymenadvance@gmail.com>
@HinataKah0HinataKah0 mentioned this pull request Jun 8, 2023
Merged
1 task
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ovflowdovflowdovflowd approved these changes
@aymen94aymen94aymen94 approved these changes
+1 more reviewer
@shanpriyanshanpriyanshanpriyan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@FHachez@ovflowd@aymen94@shanpriyan