Migrate Dropdown Component by harshitphoenix · Pull Request #5240 · nodejs/nodejs.org

harshitphoenix · 2023-04-09T06:37:11Z

Migrate Dropdown component from nodejs.dev and created a new story.

Related to: #5193

manishprivet · 2023-04-09T06:47:32Z

@@ -0,0 +1,40 @@
+import { useState, useEffect } from 'react';
+
+export interface ElementPositionAndSize {


This should go into the types folder

And probably be a type and not interface

manishprivet · 2023-04-09T06:49:19Z

@harshitphoenix Thanks for the first contribution 🙌🏼

LGTM, just added one comment. Also seems like you will have to run npm run lint and npm run build to check and fix the lint and build errors

ovflowd

Nice stuff! Amazing first contribution! Thank you 🙇

Besides of the few comments I've left, I think we're good to go (after they get addressed)

ovflowd · 2023-04-09T16:57:55Z

+import Dropdown from '..';
+
+describe('Dropdown component', () => {
+ const ref = {


Is all this extra casting needed here? I feel there are ways we could simplify the casting here 🤔 Maybe we could use the satisfy operator? https://dev.to/ayc0/typescript-49-satisfies-operator-1e4i

ovflowd · 2023-04-09T16:58:36Z

@@ -0,0 +1,30 @@
+.dropdown-list {


I think we should use camelCase classnames here

ovflowd · 2023-04-09T16:58:51Z

@@ -0,0 +1,40 @@
+import { useState, useEffect } from 'react';
+
+export interface ElementPositionAndSize {


And probably be a type and not interface

ovflowd · 2023-04-09T16:59:47Z

+ elementRef: React.RefObject<T>;
+}
+
+const Dropdown = <T extends HTMLElement>({


I think on the other components we did not destructure props on component declaration?

ovflowd · 2023-04-09T17:00:07Z

+
+ const mappedElements = items.map(item => {
+ const extraStyles = {
+ fontWeight: (item.active ? 'bold' : 'normal') as 'bold' | 'normal',


Is a casting of as bold | normal needed here?

ovflowd · 2023-04-09T17:03:43Z

@manishprivet actually the command for automatically fixing the lint errors is npm run format.

shanpriyan · 2023-04-09T19:36:18Z

Run npm run format to fix prettier lint errors

Check .eslintrc in #5236 and add it here (or may be we should raise a separate PR for that cc: @ovflowd , @manishprivet )

shanpriyan

LGTM! Thanks for the contribution ⭐️

I have left a few comments to fix the lint errors.

…omponent

ovflowd · 2023-04-17T07:49:20Z

@shanpriyan does this Dropdown component conflict with the one you created?

shanpriyan · 2023-04-18T18:38:27Z

@shanpriyan does this Dropdown component conflict with the one you created?

It's not conflicting I believe

shanpriyan · 2023-04-18T18:39:11Z

@@ -0,0 +1,30 @@
+.dropdown-list {


Suggested change
.dropdown-list {
.dropdownList {

ovflowd · 2023-04-18T20:43:55Z

But @shanpriyan you made your own Dropdown component. If that gets merged first, what are the benefits of this one? It will be a direct conflict, no?

…omponent

shanpriyan · 2023-04-19T13:53:14Z

But @shanpriyan you made your own Dropdown component. If that gets merged first, what are the benefits of this one? It will be a direct conflict, no?

@ovflowd I'm not sure which DropDown Component you are referring to, As far as I remember I have not worked on any DropDown component 😄

…omponent

ovflowd · 2023-04-19T14:25:14Z

But @shanpriyan you made your own Dropdown component. If that gets merged first, what are the benefits of this one? It will be a direct conflict, no?
@ovflowd I'm not sure which DropDown Component you are referring to, As far as I remember I have not worked on any DropDown component 😄

Actually my bad, who did another Dropdown component was @ktssr, so I wonder if we merge that one or this one. I'm not sure what the differences are.

@ktssr this PR has a Hook that calculates positioning and stuff, did you not need to add that stuff there? What are your opinions? If the language selector PR gets merged it would probably kill the reason for this PR to exist to begin with 😅

ktssr · 2023-04-19T14:51:30Z

@ovflowd, I didn't include the positioning hook in my changes because it seemed unnecessary. However, if you feel it's necessary, you can merge this PR first and I'll make necessary changes accordingly.

ovflowd · 2023-04-19T14:55:26Z

Hey @ktssr I don't know if it is necessary, and it might not be. I think we can proceed with your implementation, which is simpler and if we feel that something is missing in the future, we can always revisit the Dropdown component with what we saw in this PR.

I also feel like the author of this PR kinda abandoned the PR as he didn't reply to any of our comments yet lol

ovflowd · 2023-04-20T08:43:32Z

Closing as another Dropdown component got merged.

harshitphoenix requested a review from a team as a code owner April 9, 2023 06:37

manishprivet reviewed Apr 9, 2023
View reviewed changes

ovflowd approved these changes Apr 9, 2023
View reviewed changes

manishprivet added the website redesign Issue/PR part of the Node.js Website Redesign label Apr 9, 2023

shanpriyan reviewed Apr 9, 2023
View reviewed changes

Comment threadhooks/useElementPositionAndSize.ts

shanpriyan reviewed Apr 9, 2023
View reviewed changes

Comment threadhooks/useElementPositionAndSize.ts Outdated

shanpriyan approved these changes Apr 10, 2023
View reviewed changes

ovflowd reviewed Apr 11, 2023
View reviewed changes

Comment threadcomponents/Common/Dropdown/index.tsx Outdated

ovflowd force-pushed the major/website-redesign branch 2 times, most recently from 5cdab56 to 0240b9bCompare April 15, 2023 08:17

harshitphoenixand others added 2 commits April 15, 2023 10:25

add Dropdown component and story
85ae17f

chore: fixed linting and type errors
6306a22

ovflowd force-pushed the feature/migrate-Dropdown-Component branch from 6d49fd9 to 6306a22Compare April 15, 2023 08:28

Merge branch 'major/website-redesign' into feature/migrate-Dropdown-C…
854ec3f
…omponent

aymen94 approved these changes Apr 17, 2023
View reviewed changes

shanpriyan reviewed Apr 18, 2023
View reviewed changes

Merge branch 'major/website-redesign' into feature/migrate-Dropdown-C…
25add5b
…omponent

Merge branch 'major/website-redesign' into feature/migrate-Dropdown-C…
3cd054a
…omponent

ovflowd closed this Apr 20, 2023

		@@ -0,0 +1,40 @@
		import { useState, useEffect } from 'react';

		export interface ElementPositionAndSize {

Uh oh!

Conversation

harshitphoenix commented Apr 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manishprivet commented Apr 9, 2023

Uh oh!

ovflowd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovflowd commented Apr 9, 2023

Uh oh!

Uh oh!

Uh oh!

shanpriyan commented Apr 9, 2023

Uh oh!

shanpriyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ovflowd commented Apr 17, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shanpriyan commented Apr 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovflowd commented Apr 18, 2023

Uh oh!

shanpriyan commented Apr 19, 2023

Uh oh!

ovflowd commented Apr 19, 2023

Uh oh!

ktssr commented Apr 19, 2023

Uh oh!

ovflowd commented Apr 19, 2023

Uh oh!

ovflowd commented Apr 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ovflowd commented Apr 17, 2023•
edited
Loading