Modal showing Terms of Use and Privacy Policy acknowledgment
Design:
Design (light mode)
Error flow:
Details
Details
- Reviewers
tomek atul - Commits
- rCOMMf55db4d9d888: [native] Terms of Use & Privacy modal
- Check if links open properly
- Check if the accept button works properly
- Check if there is no possibility to close the modal in a different way than clicking I accept
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Two visual issues:
- Loading indicator isn't centered vertically
- Button height is different when loading indicator is present
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 10–11 ↗ | (On Diff #19254) | We can merge these |
| 47 ↗ | (On Diff #19254) | We're only using policyState[policyType] so we can make this more efficient by returning exactly this from selector. By doing that we can avoid unnecessary renders. |
| 72 ↗ | (On Diff #19254) | Looks like a really strange prop, especially that it is mandatory. We probably should refactor this at some point. |
| 80 ↗ | (On Diff #19254) | Are you completely sure that disabling this rule is appropriate in this case? |
| native/navigation/root-navigator.react.js | ||
| 157 ↗ | (On Diff #19254) | How about Android physical back button? |
| native/themes/colors.js | ||
| 153–154 ↗ | (On Diff #19254) | We should try to be consistent - we're using uppercase colors in this file |
Comment Actions
Fixed
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 181 | This is hacky but looks like <ActivityIndicator> behaves differently depending on the platform. We used that kind of approach to center spinner previously in codebase, eg. here. | |
| 72 ↗ | (On Diff #19254) | Changed this to not use <LoadingIndicator> which is styled for calendar utils but to use <ActivityIndicator> from react-native. |
| 80 ↗ | (On Diff #19254) | I did the same thing as register panel, but actually, I don't thing it's needed. Created a workaround instead of disabling a rule. |
| native/navigation/root-navigator.react.js | ||
| 157 ↗ | (On Diff #19254) | Previously button was hiding the modal but it was immediately shown again. To improve UX adding blocking this action using BackHandler. |
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 46 ↗ | (On Diff #19330) | How about loadingStatus? |
| 54–58 ↗ | (On Diff #19330) | We should be precise in defining dependencies. This effect doesn't depend directly on policyType so it shouldn't be included. Also it cares only about policyState?.isAcknowledged so we can depend on this (if Flow complains, we can assign it to a variable and include it in the array). |
| 55–56 ↗ | (On Diff #19330) | After we go back, this screen might remain in the tree. The problem is that we might still call this function even when this screen isn't on top, which might result in other screen becoming hidden. So maybe we should check if we're on top and only then go back? |
| 61–66 ↗ | (On Diff #19330) | Can we simplify this? |
| 83–86 ↗ | (On Diff #19330) | |
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 83–86 ↗ | (On Diff #19330) | Can this be simplified even further? const onBackPress = props.navigation.isFocused; |
Comment Actions
Just one question about re-rendering and using a hook
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 55–59 ↗ | (On Diff #19377) | We have to make sure that the component is re-rendered when a focus changes. Are we sure that using props.navigation would ensure that? Maybe it is safer to use a hook useIsFocused? |
| native/account/terms-and-privacy-modal.react.js | ||
|---|---|---|
| 55–59 ↗ | (On Diff #19377) | Your concern is totally correct. I've tested this and props.navigation isn't enough, it will not trigger useEffect while the navigation stack changes. I was in the middle of rebasing previously so I did arc diff and wanted to check if this works correctly right after and I forgot to hit Plan changes - sorry for that, I probably wasted part of your time. |