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(ui): migrate NativeSelect, NativeSelectOption and NativeSelectOptionGroup components to TypeScript #599

Merged
merged 18 commits into from
Nov 13, 2024

Conversation

guoda-puidokaite
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite commented Nov 9, 2024

Summary

  • Migrated NativeSelect, NativeSelectOption and NativeSelectOptionGroup components to TypeScript, enhancing code quality, maintainability, and scalability.
  • Utilised static typing to minimize runtime errors and improve the development experience.
  • Refactored the components for cleaner, more maintainable code.
  • Kept NativeSelect and NativeSelectOption components for use in Javascript files.
  • Note bugs and improvements found: [TASK](ui): Migrate NativeSelect(All related) component to TypeScript #240 (comment) for improvement epic.

Changes Made

TypeScript Migration:

  • Converted NativeSelect, NativeSelectOption and NativeSelectOptionGroup components files to TypeScript.
  • Added comprehensive type annotations.
  • Moved deprecated NativeSelect and NativeSelectOption components to /deprecated_js for Javascript files.
  • Exported migrated components instead of deprecated.

Refactoring:

  • Made necessary adjustments to address TypeScript-detected issues.
  • Further refactored for cleaner and more understandable code.

Testing:

  • Added edge cases:
    • Native Select
      • Without children
      • Multiple SelectOptionGroup components
      • Without value or label
    • Native Select Option
      • No value or label provided
      • Number type for value and label
    • Native Select Option Group
      • Empty label prop
      • undefined` children
      • Multiple children of different types
      • Empty component
      • Invalid props
  • Organised tests by what they check for better readability and maintenance.
  • Verified components behaviour using Storybook.
  • Updated tests to use vitest.

Documentation:

  • Updated Storybook documentation.

Release:

  • Generated a changeset file.

Related Issues

Testing Instructions

  1. Check Storybook

Checklist

  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have made corresponding changes to the documentation (if applicable).
  • My changes generate no new warnings or errors.

@guoda-puidokaite guoda-puidokaite added enhancement New feature or request package All tasks related to package under packages/ ui-components All tasks related to juno-ui-components library refactor labels Nov 9, 2024
@guoda-puidokaite guoda-puidokaite self-assigned this Nov 9, 2024
Copy link

changeset-bot bot commented Nov 9, 2024

🦋 Changeset detected

Latest commit: 441ae7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudoperators/juno-ui-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 9, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-11-13 15:14 UTC

Copy link
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

Overall, this looks great to me! I’ve added just a few comments for consideration.

andypf
andypf previously approved these changes Nov 11, 2024
Copy link
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

Great Job!

Copy link
Contributor

@barsukov barsukov left a comment

Choose a reason for hiding this comment

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

I leave it as a comment, all good few little comments left do not wanna block 🥇

andypf
andypf previously approved these changes Nov 13, 2024
Copy link
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

👍

andypf
andypf previously approved these changes Nov 13, 2024
@ArtieReus
Copy link
Collaborator

I hope this hasn’t been mentioned already, but I just noticed that the default value for NativeSelect is set to Unnamed Select instead of an empty string. Is this intentional?
Screenshot 2024-11-13 at 14 26 01

@ArtieReus
Copy link
Collaborator

I hope this hasn’t been mentioned already, but I just noticed that the default value for NativeSelect is set to Unnamed Select instead of an empty string. Is this intentional? Screenshot 2024-11-13 at 14 26 01

sorry... i just have been pointed that it is already clarified.
#599 (comment)

@ArtieReus
Copy link
Collaborator

Should the label not being defaulted to empty string?
Screenshot 2024-11-13 at 14 41 02

@guoda-puidokaite
Copy link
Contributor Author

Should the label not being defaulted to empty string? Screenshot 2024-11-13 at 14 41 02

Thanks for the review! We're not currently doing this. But I definitely think it's a good idea to introduce it, making sure it's always safely handled as a string. Good catch.

Copy link
Collaborator

@ArtieReus ArtieReus left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

LGTM

@andypf andypf merged commit c9190d0 into main Nov 13, 2024
15 checks passed
@andypf andypf deleted the guoda-convert-native-select-to-ts branch November 13, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request package All tasks related to package under packages/ refactor ui-components All tasks related to juno-ui-components library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK](ui): Migrate NativeSelect(All related) component to TypeScript
4 participants