-
Notifications
You must be signed in to change notification settings - Fork 985
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
Improve and standarize Navigation API #18703
Comments
@briansztamfater I'm adding this as an issue so that it's addressed 👍 |
@ulisesmac @brianfernalld FYI: I left navigation effects on purpose out of this PR #18047 to reduce the chance of regressions from a single PR. We have a new guideline, which I believe has not being solidified in From this guideline, we would use the keyword But anyway, just wanted to share about the PR and the guideline for effect names. |
Problem
Taken from @briansztamfater 's comment:
Description
[...] it is confusing to have navigation effects and events with same naming, like
status-mobile/src/status_im/navigation/events.cljs
Line 55 in 972a88e
status-mobile/src/status_im/navigation/effects.cljs
Line 91 in 602b271
.......
or
.......
status-mobile/src/status_im/navigation/events.cljs
Line 34 in 972a88e
status-mobile/src/status_im/navigation/effects.cljs
Line 78 in 602b271
We should rename events to have navigation / namespace prefix as per our guidelines
status-mobile/doc/new-guidelines.md
Lines 364 to 372 in 5cc027a
so events are like
:navigation/navigate-back
,:navigation/navigate-back-to
,:navigation/navigate-to
,:navigation/navigate-to-within-stack
, etc, or even better they could be improved as:navigation/pop
,:navigation/pop-to
,:navigation/push
,:navigation/push-within-stack
, etc, to comply with common nomenclature for stack navigationsThe text was updated successfully, but these errors were encountered: