feat(RandomContributor): Migrate Component #5378

Harkunwar · 2023-05-15T04:32:55Z

Description

Migrates RandomContributor from nodejs.dev

Related Issues

Check List

I have read the Contributing Guidelines and made commit messages that follow the guideline.
I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
I have run npx turbo format to ensure the code follows the style guide.
I have run npx turbo test to check if all tests are passing, and/or npx turbo test:snapshot to update snapshots if I created and/or updated React Components.
I've covered new added functionality with unit tests if necessary.

Co-authored-by: Manish Kumar ⛄ <[email protected]> Co-authored-by: Wai.Tung <[email protected]> Co-authored-by: Shanmughapriyan S <[email protected]> Co-authored-by: Michael Esteban <[email protected]> Co-authored-by: Claudio Wunder <[email protected]> Co-authored-by: Teja Sai Sandeep Reddy Konala <[email protected]> Co-authored-by: Claudio Wunder <[email protected]> Co-authored-by: vasanth9 <cheepurupalli.vasanthkumar.com> Co-authored-by: Aymen Naghmouchi <[email protected]> Co-authored-by: Teja Sai Sandeep Reddy Konala <[email protected]> Co-authored-by: Augustin Mauroy <[email protected]> Co-authored-by: Guilherme Araújo <[email protected]> Co-authored-by: Augustin Mauroy <[email protected]> Co-authored-by: HinataKah0 <[email protected]> Co-authored-by: Olaleye Blessing <[email protected]> Co-authored-by: ktssr <[email protected]> Co-authored-by: vasanthkumar <[email protected]> Co-authored-by: Floran Hachez <[email protected]> Co-authored-by: Jatin <[email protected]> fixed styleling misconfig and fixed storybooks (nodejs#5281) fix storybook styles, imports, typescript config and dependencies (nodejs#5319 fix(package.json) Lint command is missing slashes (nodejs#5321 fix storybook local development mode (nodejs#5335) fix(i18n): translation key (nodejs#5347)

… method (nodejs#5369) Add clarity to step 4 based on clone method Signed-off-by: Vessy Shestorkina <[email protected]>

* Migrate JsonLink component Signed-off-by: Vessy Shestorkina <[email protected]> * Add storybook snap and robot icon Signed-off-by: Vessy Shestorkina <[email protected]> * Remove IconContext provider Signed-off-by: Vessy Shestorkina <[email protected]> --------- Signed-off-by: Vessy Shestorkina <[email protected]> Signed-off-by: Claudio Wunder <[email protected]> Co-authored-by: Claudio Wunder <[email protected]>

…s#5376) * refactor(useDownloadLink): Move useDetectOs to useDownloadLink * refactor(useDownloadLink): rename file * refactor(useDownloadLink): rename file * Apply suggestions from code review Signed-off-by: Claudio Wunder <[email protected]> * refactor(useDownloadLink): undo deleted file --------- Signed-off-by: Claudio Wunder <[email protected]> Co-authored-by: Claudio Wunder <[email protected]>

vercel · 2023-05-15T04:32:59Z

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

Name	Status	Preview	Comments	Updated (UTC)
nodejs-org	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 15, 2023 5:03am
nodejs-org-stories	✅ Ready (Inspect)	Visit Preview		May 15, 2023 5:03am

ovflowd

Hey, @Harkunwar I super appreciate for taking the migration of that component :)

I left a lot of comments, just to raise the bar of the quality of the PR. Regardless, thank you so much for working on this!

ovflowd · 2023-05-15T05:19:48Z

components/Common/RandomContributor/index.module.scss

@@ -0,0 +1,27 @@
+.random-contributor {


We usually use camelCase classnames

ovflowd · 2023-05-15T05:20:02Z

components/Common/RandomContributor/index.module.scss

+  max-width: 400px;
+  padding: var(--space-12);
+
+  @media (max-width: 900px) {


I think we somewhere added a mixin for the 900px max-wdith

ovflowd · 2023-05-15T05:20:39Z

components/Common/RandomContributor/index.stories.tsx

+  parameters: {
+    moduleMock: {
+      mock: () => {
+        const mock = createMock(hooks, 'useNodeJsContributorsApi');


Can we call the hook useFetchNodeContributor?

ovflowd · 2023-05-15T05:21:19Z

components/Common/RandomContributor/index.tsx

+
+  return (
+    <div ref={ref} className={styles.randomContributor}>
+      {!contributor && isVisible && <AnimatedPlaceholder />}


We can leave the animated placeholder even when it's not visible, to reduce layout shift.

Suggested change

{!contributor && isVisible && <AnimatedPlaceholder />}

{contributor || <AnimatedPlaceholder />}

ovflowd · 2023-05-15T05:22:38Z

components/Common/RandomContributor/index.tsx

+import { useOnScreen } from '../../../hooks/useOnScreen';
+import { useNodeJsContributorsApi } from '../../../hooks/useNodeJsContributorsApi';
+
+const DefaultImageSize = 75;


Constants are usually uppercase

Suggested change

const DefaultImageSize = 75;

const DEFAULT_IMAGE_SIZE = 75;

ovflowd · 2023-05-15T05:26:58Z

hooks/__tests__/useOnScreen.test.tsx

This whole test feels overcomplicated imho, feels like we're testing things that do not need to be tested. But I'm not sure. This has a chance to become a flaky test

Yeah I don't think it's easy to test this one since we're basically mocking everything.

ovflowd · 2023-05-15T05:28:02Z

hooks/useNodeJsContributorsApi.tsx

+  page: number;
+};
+
+export type ContributorApiResponse = {


We don't need a type that covers the full JSON response, we can simply just leave what we want.

It is indeed a partial type that wouldn't reflect 100% on the extent of the object, but I rather doing that than keeping a bunch of types that have no value for me.

ovflowd · 2023-05-15T05:28:44Z

hooks/useNodeJsContributorsApi.tsx

+import { NodeJSContributorsApiKey } from '../constants/swr';
+
+const LIMIT_CONTRIBUTORS = 5;
+const CONTRIBUTORS_API_URI = `https://api.github.com/repos/nodejs/node/contributors?per_page=${LIMIT_CONTRIBUTORS}`;


I feel we could have a dedicated constants file for all these sorts of constants, including API URL constants as we had on nodejs.dev

(To have a single point for all constants, which makes it easier to find)

ovflowd · 2023-05-15T05:32:10Z

hooks/useNodeJsContributorsApi.tsx

Sorry, but I have a single word for this Hook. Megazord.

It is doing way way many things. A React Hook should never be this much convoluted and complicated.

It is breaking the single responsibility pattern

It mixes all sort of design patterns making it confusing

It's convoluted and big

Hard to heard

Newcomer unfriendly

Let's, please simplify the logic, document what is possible to be documented, outsource actual utilities such as link parsing and etc, to somewhere (we want to be able to reuse these bits of code as much as possible in different places), and keep the Hook real and simple

ovflowd · 2023-05-15T05:34:57Z

package.json

@@ -68,6 +68,7 @@
    "@storybook/addon-interactions": "^7.0.8",
    "@storybook/addon-links": "^7.0.8",
    "@storybook/blocks": "^7.0.8",
+    "@storybook/jest": "^0.1.0",


How often is this module maintained?

I'm worried that we're needlessly more and more adding packages to our website. I think, for now, it's okay, but let's keep this in mind. We don't want to have too many packages accidentally by the end of the day...

Let's keep this on mind!

This module seems to be actively maintained. The last version about a month old.

Harkunwar · 2023-05-15T05:54:12Z

Thanks for the reviews @ovflowd . I tried migrating with minimal modifications to the original code. I'll address all the issues and bring it up to better standards.

ovflowd · 2023-05-15T05:54:57Z

Thank you, @Harkunwar! 🙇

AugustinMauroy · 2023-05-17T13:50:31Z

We should put on home component ?

ovflowd · 2023-05-24T13:39:20Z

FYI: We deleted the major/website-redesign branch to simplify our release and rollout process. Please feel free to recreate this PR or reopen it with the main branch as a target.

Thank you, and we apologise for the inconvenience!

Harkunwar and others added 9 commits May 5, 2023 10:09

chore: no-unused-vars ignores pattern starting with _ (nodejs#5363)

c526a02

Contributing Guidelines Update - Add clarity to step 4 based on clone…

566f3d9

… method (nodejs#5369) Add clarity to step 4 based on clone method Signed-off-by: Vessy Shestorkina <[email protected]>

fix(test): dir name (nodejs#5354)

49efa04

feat(stability): migrate component (nodejs#5339)

a51457d

feat(RandomContributor): Migrate component

21bbfb0

feat(RandomContributor): Add snapshot

0431737

vercel bot temporarily deployed to Preview – nodejs-org-stories May 15, 2023 04:34 Inactive

Harkunwar added 2 commits May 14, 2023 21:35

feat(RandomContributor): Add constant for image size

a418f30

feat(RandomContributor): Fix default image url + snapshot

8982050

vercel bot temporarily deployed to Preview – nodejs-org-stories May 15, 2023 04:40 Inactive

vercel bot temporarily deployed to Preview – nodejs-org May 15, 2023 04:42 Inactive

Harkunwar requested review from ovflowd and shanpriyan May 15, 2023 04:42

Harkunwar marked this pull request as ready for review May 15, 2023 04:42

feat(RandomContributor): Add loading story

e744dd2

vercel bot temporarily deployed to Preview – nodejs-org May 15, 2023 05:02 Inactive

vercel bot temporarily deployed to Preview – nodejs-org-stories May 15, 2023 05:03 Inactive

ovflowd requested changes May 15, 2023

View reviewed changes

HinataKah0 mentioned this pull request May 16, 2023

chore: consolidate Node releases data #5365

Merged

1 task

ovflowd force-pushed the major/website-redesign branch from cb8c95e to 9243764 Compare May 24, 2023 13:33

ovflowd requested a review from a team as a code owner May 24, 2023 13:33

ovflowd deleted the branch nodejs:major/website-redesign May 24, 2023 13:37

ovflowd closed this May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(RandomContributor): Migrate Component #5378

feat(RandomContributor): Migrate Component #5378

Harkunwar commented May 15, 2023

vercel bot commented May 15, 2023 •

edited

Loading

ovflowd left a comment

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

Harkunwar May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

ovflowd May 15, 2023

Harkunwar May 15, 2023

Harkunwar commented May 15, 2023

ovflowd commented May 15, 2023

AugustinMauroy commented May 17, 2023

ovflowd commented May 24, 2023

	{!contributor && isVisible && <AnimatedPlaceholder />}
	{contributor \|\| <AnimatedPlaceholder />}

feat(RandomContributor): Migrate Component #5378

feat(RandomContributor): Migrate Component #5378

Conversation

Harkunwar commented May 15, 2023

Description

Related Issues

Check List

vercel bot commented May 15, 2023 • edited Loading

ovflowd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harkunwar commented May 15, 2023

ovflowd commented May 15, 2023

AugustinMauroy commented May 17, 2023

ovflowd commented May 24, 2023

vercel bot commented May 15, 2023 •

edited

Loading