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.

Sign up for GitHub

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

Jump to bottom

chore(ui): migrate DateTimePicker to TypeScript #640

Merged
merged 23 commits into from
Dec 4, 2024
Merged

chore(ui): migrate DateTimePicker to TypeScript #640

merged 23 commits into from
Dec 4, 2024

Conversation

gjaskiewicz
Copy link
Contributor

@gjaskiewicz gjaskiewicz commented Nov 26, 2024 •
edited by andypf
Loading

Summary

migrate DateTimePicker to TypeScript

Changes Made

  • migrate DateTimePicker to TypeScript

Related Issues

Testing Instructions

  1. npm i
  2. npm run 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.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 26, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 99eb95a

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 26, 2024 •
edited
Loading

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-12-04 15:44 UTC

@ArtieReus ArtieReus added the ui-components All tasks related to juno-ui-components library label Nov 27, 2024
@guoda-puidokaite guoda-puidokaite added package All tasks related to package under packages/ enhancement New feature or request labels Nov 27, 2024
@gjaskiewicz gjaskiewicz marked this pull request as ready for review November 27, 2024 10:14
@gjaskiewicz gjaskiewicz requested review from franzheidl and a team as code owners November 27, 2024 10:14
guoda-puidokaite
guoda-puidokaite requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice job! So many lines in the component. 🚀
Added a few blocking improvements.

packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
guoda-puidokaite
guoda-puidokaite requested changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite 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 the changes! 🚀 Just a few blocking changes again.

packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
guoda-puidokaite
guoda-puidokaite requested changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment

Choose a reason for hiding this comment

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

Requesting changes for above

@guoda-puidokaite guoda-puidokaite mentioned this pull request Nov 28, 2024
Open
27 tasks
guoda-puidokaite
guoda-puidokaite requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment

Choose a reason for hiding this comment

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

Let's double check backward compatibility for all props

packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
andypf
andypf previously approved these changes Nov 29, 2024
View reviewed changes
Copy link
Collaborator

@andypf andypf left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! However, please make sure to address the issues identified by Guoda.

@andypf andypf self-requested a review November 29, 2024 08:51
@andypf andypf dismissed their stale review November 29, 2024 08:52

please make sure to address the issues identified by Guoda.

guoda-puidokaite
guoda-puidokaite requested changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment

Choose a reason for hiding this comment

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

🙏

packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
barsukov
barsukov requested changes Dec 3, 2024
View reviewed changes
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 left few comments please answer them and fix some of them the main issue is with this method updateFlatpickrInstance you modified which could produce a potential bugs or fix :)

packages/ui-components/src/components/DateTimePicker/DateTimePicker.test.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.test.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Show resolved Hide resolved
packages/ui-components/src/components/DateTimePicker/DateTimePicker.component.tsx Outdated Show resolved Hide resolved
@gjaskiewicz gjaskiewicz requested a review from barsukov December 3, 2024 13:30
guoda-puidokaite
guoda-puidokaite requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(Regression? Please double check...)

In the PR Storybook (right), it seems to take two clicks to set or change a date compared to live (left).
Maybe something changed regarding state setting or so?
Could you double check 1. whether you can recreate this 2. why it's happening and fix it. @gjaskiewicz

Screen.Recording.2024-12-03.at.16.33.15.mov

guoda-puidokaite
guoda-puidokaite reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(Out-Of-Scope Change)

Default has been changed. Looks fine, but can we keep everything the same?

Screenshot 2024-12-04 at 09 32 57

@guoda-puidokaite
Copy link
Contributor

(Requested Improvement)

For all deprecated props, could you mention something along the lines of Some prop types have been deprecated: Use x and y for future compatibility. As this current message is usually for a deprecated component.

Screenshot 2024-12-04 at 10 00 14 Screenshot 2024-12-04 at 10 01 11

guoda-puidokaite
guoda-puidokaite approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@guoda-puidokaite guoda-puidokaite left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Regression fixed, Thank You.
Types look good. I've added types for this component to future improvements. Can be re-addressed there.

@barsukov barsukov merged commit e96f612 into main Dec 4, 2024
15 checks passed
@barsukov barsukov deleted the gjaskiewicz/refactor-dtp-ts branch December 4, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guoda-puidokaite guoda-puidokaite guoda-puidokaite approved these changes

@barsukov barsukov barsukov approved these changes

@franzheidl franzheidl Awaiting requested review from franzheidl franzheidl is a code owner

@andypf andypf Awaiting requested review from andypf

Assignees

@gjaskiewicz gjaskiewicz

Labels
enhancement New feature or request package All tasks related to package under packages/ ui-components All tasks related to juno-ui-components library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gjaskiewicz @guoda-puidokaite @barsukov @andypf @ArtieReus