Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: migrate Dropdown to TS #1976

Merged
merged 18 commits into from
May 19, 2024
Merged

chore: migrate Dropdown to TS #1976

merged 18 commits into from
May 19, 2024

Conversation

Mila2999
Copy link
Contributor

@Mila2999 Mila2999 commented Feb 19, 2024

fixes #1622

Basic

  • Used plop (npm run plop) to create a new component.
  • PR has description.
  • New component is functional and uses Hooks.
  • Component is written in TypeScript.

Style

  • Styles are added to NewComponent.modules.scss file inside the NewComponent folder.
  • Component uses CSS Modules.
  • Design is compatible with Monday Design System.
  • Styles are defined using monday-ui-style tokens
  • Displays correctly in all the themes

Storybook

  • Stories were added to /src/components/NewComponent/__stories__/NewComponent.mdx file.
  • Stories include all flows of using the component.
  • Stories implemented using vibe-storybook-components components and tokens.

Tests

Accessibility

@Mila2999 Mila2999 requested a review from a team as a code owner February 19, 2024 17:58
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Thank you @Mila2999! That's really great. Let's just avoid using any and leverage TS and use the actual types

@Mila2999
Copy link
Contributor Author

thanks @talkor, i will work on fixing the types

@Mila2999
Copy link
Contributor Author

Hi @talkor , i removed all any references except ref={ref as React.RefObject<any>} . any idea what can i assign to ref?

@talkor talkor changed the title chore: migrated Dropdown component mondaycom #1622 chore: migrate Dropdown to TS Mar 24, 2024
@talkor talkor self-requested a review March 31, 2024 13:38
Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to work on this! And apologies for the long delay. I wrote some comments and some notes to make this migration without breaking existing usages
Thanks!

packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/package.json Outdated Show resolved Hide resolved
packages/core/package.json Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
@Mila2999
Copy link
Contributor Author

Mila2999 commented Apr 5, 2024

Hi @talkor , thank you for your time in reviewing my PR. I have addressed your comments and made the necessary fixes to the code

@Mila2999
Copy link
Contributor Author

@talkor i reverted react-windowed-select from 3.1.0 back to "^2.0.4". to fix typescript errors i added a file and declared a module there, this fixed errors in vs code for me.

Copy link
Contributor

@YossiSaadi YossiSaadi left a comment

Choose a reason for hiding this comment

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

You did amazing @Mila2999 !
Thank you! 🙏🏼

I went through mainly on the props, will go through the rest of stuff in another iteration

@YossiSaadi
Copy link
Contributor

Hey @Mila2999 !
We would like to merge this PR as-is :)
we have fixes for mine and @talkor's comments in a different branch
you really did brilliant job!
would you mind fix the conflict with master so we can merge it?

@Mila2999
Copy link
Contributor Author

Hi @YossiSaadi,

Thank you! I appreciate the feedback. I have rebased my code onto the master branch and resolved the conflicts in the latest commit. Sorry for any delays, as I'm currently on vacation, but I really would like to finalize this issue.

@talkor
Copy link
Member

talkor commented May 19, 2024

Hi @YossiSaadi,

Thank you! I appreciate the feedback. I have rebased my code onto the master branch and resolved the conflicts in the latest commit. Sorry for any delays, as I'm currently on vacation, but I really would like to finalize this issue.

@Mila2999 thank you so much for the time you put into it! We really appreciate it! 🙏🏼

@talkor talkor merged commit ef4f96b into mondaycom:master May 19, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS-migration: Dropdown
3 participants