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

feat: add date control type #223

Merged
merged 5 commits into from
Jul 14, 2021
Merged

feat: add date control type #223

merged 5 commits into from
Jul 14, 2021

Conversation

dannyhw
Copy link
Member

@dannyhw dannyhw commented Jul 13, 2021

Issue: #199

What I did

I added the date control type

I also updated the date picker library to resolve the yellow warnings it triggered previously and fix the web version of this control.

How to test

First make sure you run yarn install or yarn bootstrap in the root and then run pod install in the example app

  • Open the example app
  • open the date story
  • Adjust the date by using the control in the addons tab

@dannyhw dannyhw added the 6.5 label Jul 13, 2021
@dannyhw dannyhw added this to the v6 alpha 1 milestone Jul 13, 2021
@dannyhw dannyhw self-assigned this Jul 13, 2021
};
}
const DateType = ({ onChange, arg: { name, value } }: DateProps) => {
const [isDateVisible, setIsDateVisible] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This split to two different boolean variables (isDateVisible and isTimeVisible) pre-dates your code, but I'm tempted to suggest a small refactor here: #225

It's by no means a priority or a blocker, but thought you might be able to take a quick peek now that this code is fresh in the minds of both of us.

Copy link
Member Author

Choose a reason for hiding this comment

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

accepted your PR, seems like a great idea to me. Thanks again!

Copy link
Contributor

@lauriharpf lauriharpf left a comment

Choose a reason for hiding this comment

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

Again superb work! 🚀 💪 💎

Some minor comments, take a peek if you have the time for it. In general the control seems to work very well and the conversion from the knob looks smooth 👍.

dannyhw and others added 4 commits July 14, 2021 20:37
add time to date example

Co-authored-by: Lauri Harpf <[email protected]>
…an values ("isDateVisible" and "isTimeVisible") allow for one invalid state (both shouldn't be true at the same time, as that would mean both pickers are visible simultaneously, which can't happen with the current implementation). (#225)
@dannyhw dannyhw merged commit 22d52ac into next-6.0 Jul 14, 2021
@dannyhw dannyhw deleted the feat/date-control branch July 14, 2021 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants