-
-
Notifications
You must be signed in to change notification settings - Fork 413
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 set first day week android #902
Merged
vonovak
merged 14 commits into
react-native-datetimepicker:master
from
JoshBarnesD:feat/set-first-day-week-android
Jun 3, 2024
Merged
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
22d4805
Added implementation for firstDayOfWeek for Android
JoshBarnesD c240ded
Fixed failing tests due to changes in App.js and fixed inconsistent t…
JoshBarnesD e2cbf3d
Added e2e tests for firstDayOfWeek Android
JoshBarnesD db0a276
Fixed .java files formatting issues due to changes
JoshBarnesD 9f8f3c9
Fixed .java files formatting issues due to changes (2)
JoshBarnesD a20de55
Fixed failing test on iOS
JoshBarnesD 43a0511
Added descriptions in type files and checked SDK version compatability
JoshBarnesD 6858793
Updated firstDayOfWeek in README.md
JoshBarnesD 9759027
Modified descriptions in type files
JoshBarnesD f344787
Fixed failing e2e tests on Android
JoshBarnesD 46a4888
Fixed failing e2e tests on iOS
JoshBarnesD 8d05525
Fixed failing e2e tests on iOS (2)
JoshBarnesD 11ea194
README.md changes as requested, detox tests modified to reduce repeti…
JoshBarnesD 0f9786b
Added helper function userSwipesTimezoneListUntilDesiredIsVisible
JoshBarnesD File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned you needed to add scrolling. Can we make this more compact in terms of height?
I'd prefer if we can avoid scrolling because it's annoying to think about it when maintaining tests.
Also the code seems fit to be replaced by a
array.map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, thank you for the lovely comments you've made, they made my day! 😄
Now, with regards to this, Ive implemented a FlatList design for selecting the
DAY_OF_WEEK
variable (similar to the "timezone" FlatList further down) but, sadly, it seems most of the tests would still need to have some level of scrolling to access the UI elements... Either way I will push the changes shortly and you can check them out!I've gone through most of the tests and I think it might be reasonable to implement a "ScrollToElement" function that allows you to scroll to the necessary UI element before interacting with it. This would also reduce the size and repetitiveness of some of the tests and hopefully aid with maintainability. I was thinking of maybe creating a separate PR going over them and addressing some of the issues. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, though if we can avoid scrolling, that'd be best imho. I see that now we run in iphone 14. A low-effort fix could be just bumping to a larger device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revert the changes to the tests that add scrolling and see if I can find a valid device for iOS and Android that will pass the tests without scrolling, will get back to you shortly