Skip to content

Add Windows date picker - #157

Merged
vonovak merged 6 commits into
react-native-datetimepicker:masterfrom
rectified95:master
May 14, 2020
Merged

Add Windows date picker#157
vonovak merged 6 commits into
react-native-datetimepicker:masterfrom
rectified95:master

Conversation

@rectified95

@rectified95rectified95 commented May 2, 2020

Copy link
Copy Markdown
Collaborator

Summary

Add Windows date picker. Closesmicrosoft/react-native-windows#4156
This PR extracts the DatePicker from React Native Windows in line with the lean core effort. It preserves the existing functionality available on Windows, rewriting the code using C++/WinRT to follow the latest recommended development practices.
Time picker will be added in a separate PR (tracked by microsoft/react-native-windows#4695)

Test Plan

What's required for testing (prerequisites)?

Run the example app:
0. Install system requirements: https://microsoft.github.io/react-native-windows/docs/next/rnw-dependencies

  1. Run'npm run start:windows
  2. open example/windows/datePickerTest.sln in Visual Studio and run.

Or create your own test app and link this module:
https://microsoft.github.io/react-native-windows/docs/next/native-modules-setup#option-1-create-a-new-test-app

image

Compatibility

OSImplemented
Windows

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md (doing)
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

@kaiguokaiguo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you also update the README to include instructions about linking and running example app 😊?

Comment threadexample/App.js Outdated
Comment threadexample/windows/.gitignore
Comment threadexample/windows/datePickerTest/Package.appxmanifest Outdated
Comment threadexample/windows/datePickerTest/ReactPackageProvider.cpp
Comment threadexample/windows/datePickerTest/pch.h
Comment threadwindows/DateTimePickerWindows/DateTimePickerView.cpp
Comment threadwindows/DateTimePickerWindows/DateTimePickerView.h Outdated
Comment threadwindows/DateTimePickerWindows/DateTimePickerViewManager.cpp
Comment threadwindows/DateTimePickerWindows/DateTimePickerWindows.vcxproj Outdated
Comment threadwindows/DateTimePickerWindows/readme.txt Outdated
Comment threadpackage.json
Comment threadsrc/datetimepicker.windows.js
Comment threadsrc/datetimepicker.windows.js Outdated
@rectified95

Copy link
Copy Markdown
CollaboratorAuthor

@vonovak
Looking at CHANGELOG.md, I see that not all commits are recorded there.
I still guess that I should mention this change - shall I put it under 2.3.2, bump the version to 2.3.3, or are you responsible for versioning?

Also, are you the only one who can merge this PR?

@vonovak

Copy link
Copy Markdown
Member

@rectified95 yes, the changelog is for now edited manually, I'd like to automate releases at some point soon but we're not there yet.

I think you don't need to worry about the changelog, I'll put it in there. I'm not the only one who can merge PRs but I have been the most active one here recently it seems.

Regarding maintenance, do you have an example somewhere of how this functionality can be tested? Preferably end-to-end; thank you! Would the author or someone from the reviewers be open to becoming a maintainer? (for the windows part at least, but for I'd be very open to improvements outside of windows too) Thank you!

Comment threadexample/App.js
Comment threadsrc/datetimepicker.windows.js Outdated
Comment threadmetro.config.windows.js Outdated
@rectified95

rectified95 commented May 5, 2020

Copy link
Copy Markdown
CollaboratorAuthor

@vonovak Thanks for your comments.
About testing, you can run the test app on Windows as outlined in the PR description, or set up a new test app yourself. E2E tests are being worked out and can be added as a follow-up. For now, I'd suggest just running the example app.

We'll get back to you about becoming maintainers; for now I can say that I'll fix bugs we spot after the merge.

@kaiguokaiguo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

:shipit:

Comment threadREADME.md Outdated
Comment threadREADME.md
Comment threadREADME.md
Comment threadREADME.md Outdated
Comment threadwindows/DateTimePickerWindows/DateTimePickerWindows.vcxproj Outdated
@vonovak

Copy link
Copy Markdown
Member

hello guys, please ping me when this is done, just so I know explicitly that the review is done on your side, thank you!

Comment threadexample/App.js
@@ -0,0 +1,68 @@
/**
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This came up in another repo. Not sure it's kosher to put msft copyright in a community repo we don't matinain.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Alex/Kai told me in an earlier comment to add it to all these files. Can you link the discussion you're referencing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

They'll likely know better on this. IIRC there was one community repo that asked that copyright headers be removed. @kaiguo do you remember the story there?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm, I guess that's a @chrisglein question, should we put Microsoft copyright info in a community repo we don't own?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where we landed for the first couple repos (camera, video, etc.) is to leave it off. We match the pattern of the target repo. I'll check offline, but for now assume that's the case.

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Sounds good, the Android/iOS files in this repo have the MIT license from Facebook, so I'll keep ours for now.

'RNDateTimePickerWindows',
);

export default class DateTimePickerWindows extends React.Component<WindowsNativeProps> {

@NickGerlemanNickGerlemanMay 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name WindowsNativeProps causes a bit of confusion for me. I've previously seen "native component properties" refer to the props being passed to RNDateTimePickerWindows instead of DateTimePickerWindows, since the former is considered the native component.

Future versions of Flow + TS types parameterize requireNativeComponent to take props as well which would likely take a NativeProps type.

@asklarasklar left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Filed #161 to track the alignment of the props/API with Android/iOS as a follow up. There's also follow up work to expose the calendar identifier and the fact that globalization doesn't seem to quite work yet
Please see if we can make the min version 16299 to work on RS3.

Comment threadsrc/datetimepicker.windows.js Outdated
Comment threadpackage.json
"start": "react-native start",
"start:android": "react-native run-android --no-jetifier",
"start:ios": "react-native run-ios",
"start:windows": "react-native start --use-react-native-windows",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mentioned somthing in another comment, but can we share the metro config and use run-windows?

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Not sure, but I think we still need to swap the metro config

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@acoates-ms did some work to allow a single metro config allowing multiple out of tree platforms for 0.62. Would you mind checking with him? Want to make sure we set the right patterns for future community modules based on 0.62.

@rectified95

Copy link
Copy Markdown
CollaboratorAuthor

@vonovak My colleagues have signed off on the PR.

I've also tried to set up a build workflow, but it seems like I don't have the privileges, so I've reverted it (that's what the text says if you click the 'Actions' tab in this repo). We can coordinate this as a separate PR.
Also, I want to reiterate that I'll fix bugs related to this check-in. :)

@vonovak
vonovak merged commit 27a923f into react-native-datetimepicker:masterMay 14, 2020
@vonovak

Copy link
Copy Markdown
Member

Thanks so much for this!

I've also tried to set up a build workflow, but it seems like I don't have the privileges, so I've reverted it (that's what the text says if you click the 'Actions' tab in this repo)

that'd be nice to have. But for this repo, we're not using github actions but circleCI. Since circle runs for PRs too, I'm guessing you should be able to make changes to the config and see the results in a PR.

Also, I want to reiterate that I'll fix bugs related to this check-in. :)

great, let me know if I can help

@rectified95

rectified95 commented May 14, 2020

Copy link
Copy Markdown
CollaboratorAuthor

@vonovak Thanks for being responsive and merging this! :)

There have been issues with certificates for release builds on Windows VMs in CircleCI. A colleague ran into this when adding a CI loop for the WebView module, and ended up having to use Github actions - for WebView they run alongside the CircleCI steps; is there a reason we cannot add GitHub actions for this project?
For reference: react-native-webview/react-native-webview#1358
Failed CI build: https://app.circleci.com/pipelines/github/kaiguo/react-native-webview/49/workflows/ed9e57e8-059d-46e8-a246-90d8bb21312e/jobs/124

@vonovak

Copy link
Copy Markdown
Member

hi @rectified95

There have been issues with certificates for release builds on Windows VMs in CircleCI.

okay, I had no idea about that

is there a reason we cannot add GitHub actions for this project?

I wanted to prefer using circle because we already use it so there is no entry barrier, but if there is a blocker there, I'm def not opposed to using actions too. I can give whoever wants to do this the necessary access rights.

@rectified95

Copy link
Copy Markdown
CollaboratorAuthor

@vonovak You can give me the permissions, I'll try setting it up.

Sign up for freeto 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.

Support <react-native-community/datetimepicker>

6 participants

@rectified95@vonovak@NickGerleman@kaiguo@asklar@chrisglein