Error alert after navigating to external links from app #20177 #20193
Conversation
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
Jenkins BuildsClick to see older builds (6)
|
mmilad75
force-pushed
the
milad/20177-fix-error-alert-in-open-external-link
branch
from
1e46017
to
fc3c96b
Compare
J-Son89
reviewed
May 27, 2024
| (.openURL ^js react/linking (url/normalize-url url)))))) | ||
| (do | ||
| (.openURL ^js react/linking (url/normalize-url url)) | ||
| nil))))) |
There was a problem hiding this comment.
I wonder do we need a comment or even to put nil in an explanatory def 🤔
There was a problem hiding this comment.
Returning an map/nil is part of the declaration of events. I don't think it's needed 🤔 But let me know if you want me to add it
There was a problem hiding this comment.
Makes sense. Np for me either way, just wanted to raise the question. 👍
There was a problem hiding this comment.
openURL is a function with side-effects and shouldn't be part of the event handler. There should be a re-frame effect wrapping this, so that we could do:
{:fx [...
[:linking/open-url (url/normalize-url url)]]}Probably better would be to also put the normalize-url call inside the effect since it's probably always desirable as far as I checked the code now. With the effect we won't have to worry about returning nil.
The new effect could be like this:
{:fx [...
[:linking/open-url [url {:normalize? true}]]
...]}There was a problem hiding this comment.
@ilmotta Done. Please check again
There was a problem hiding this comment.
Nice @mmilad75, LGTM, thanks!
J-Son89
approved these changes
May 27, 2024
ilmotta
approved these changes
May 27, 2024
83% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (43)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
60% of end-end tests have passedFailed tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
|
|
hi @mmilad75 Thank you for PR. No issues from my side. Ready to be merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #20177
The issue was because the event was void, which should've returned an object. But in this case, since there were no objects required to be returned, I added nil as the return value
Screen.Recording.2024-05-27.at.16.27.46.mov