-
Notifications
You must be signed in to change notification settings - Fork 9
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!: view callback and layout functionality improvements #239
feat!: view callback and layout functionality improvements #239
Conversation
catalystInstance.callFunction(Constants.NAV_JAVASCRIPT_FLAG, "onTurnByTurn", params); | ||
WritableNativeArray params = new WritableNativeArray(); | ||
params.pushMap(map); | ||
catalystInstance.callFunction(Constants.NAV_JAVASCRIPT_FLAG, "onTurnByTurn", params); |
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.
Nit: For future PRs, can we please separate changes that are meant to be mere formatting/styling changes? This will make it easier for reviewers to focus on the functional changes
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.
Yes, the project currently lacks formatting configuration, and as I locally have autoformatting, missed these changes. Once the formatting setup is complete and integrated into CI and lefthook linting, these formatting commits should disappear.
In the meantime, you can Hide whitespace
changes in the GitHub review to focus on meaningful differences.
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.
Removed formatting changes on this file for easier review.
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.
To avoid these in future, I added forced formatting to the repo for future PR's in separe PR visible here: #240
android/src/main/java/com/google/android/react/navsdk/NavViewFragment.java
Outdated
Show resolved
Hide resolved
android/src/main/java/com/google/android/react/navsdk/NavViewFragment.java
Outdated
Show resolved
Hide resolved
Nit: For future PRs that aren't too critical to get out, is it possible to stat incrementally adding unit test coverage along with our changes? |
This is a great improvement, thanks Joonas! |
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.
Bit hard to review as the formatting is in the same commit with the changes. Despite that seems good overall. I'll still check one more time before approving.
Most definetely yes. |
b3c2fe7
to
7322cc0
Compare
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.
LGTM!
7322cc0
to
a416e11
Compare
Fixes the
NavigationView
callback registration issue when using multiple view instances.Resolves navigation view scaling issues by removing the mandatory
height
andwidth
properties and adding support for an optionalstyle
property (breaking change). This allows developers to use flex or width/height parameters with percentage values.Fixes #28
Fixes #192
Fixes #233
Partially fixes #235
BREAKING CHANGE: The
height
andwidth
properties have been removed fromNavigationView
in favor of an optionalstyle
property.