feat: [#173248858] "Update IO" CTA web URL fallback added + fixed component method #2491
Conversation
DavideValdo
requested review from
CrisTofani,
fabriziofff and
Undermaken
as code owners
pagopa-github-bot
changed the title
Affected stories
|
Codecov Report
@@ Coverage Diff @@
## master #2491 +/- ##
=======================================
Coverage 50.64% 50.64%
=======================================
Files 708 708
Lines 20156 20157 +1
Branches 3878 3880 +2
=======================================
+ Hits 10208 10209 +1
Misses 9902 9902
Partials 46 46
Continue to review full report at Codecov.
|
ts/screens/modal/UpdateAppModal.tsx
Outdated
| }); | ||
|
|
||
| // Play/App store native URL | ||
| // eslint-disable-next-line |
There was a problem hiding this comment.
We may avoid to disable linting here just refactor below lines of code as follows:
// Play/App store native URL
Linking.openURL(storeUrl)
// Try to fallback to the web URL
.catch(() => Linking.openURL(webStoreURL))
// No URL could be opened, show an error message
.catch(() =>
this.setError()
// Hide the error after 5 seconds
.then(() =>
setTimeout(
() =>
this.setState({
hasError: false
}),
timeoutErrorMsg
)
)
);
ts/screens/modal/UpdateAppModal.tsx
Outdated
| public componentDidMount() { | ||
| BackHandler.addEventListener("hardwareBackPress", this.handleBackPress); | ||
| } | ||
| const AppleFooter: FC<FooterProps> = ({ onOpenAppStore }: FooterProps) => ( |
There was a problem hiding this comment.
Maybe renaming this to IosFooter is better what do you think?
There was a problem hiding this comment.
Can't say I like it, "iOS" is the correct spelling, not "Ios", but if you've chosen "Ios" all over the codebase let's give priority to coherence.
ts/screens/modal/UpdateAppModal.tsx
Outdated
| android: this.renderAndroidFooter() | ||
| }); | ||
| } | ||
| useEffect(() => { |
There was a problem hiding this comment.
We already have a common hook for this case useHardwareBackButton let's try this one 😄
ts/screens/modal/UpdateAppModal.tsx
Outdated
|
|
||
| useEffect(() => { | ||
| if (error) { | ||
| setTimeout(() => setError(false), ERROR_MESSAGE_TIMEOUT); |
There was a problem hiding this comment.
Please handle the clear of this setTimeout to avoid possible warnings
ts/screens/modal/UpdateAppModal.tsx
Outdated
| setError(true); | ||
| } | ||
| } | ||
| }, [setError]); |
There was a problem hiding this comment.
imho useCallback in this case maybe not so useful, since we are passing setError function as a dependency this would never be regenerated and anyway here there's no need to memoize the function.
I would suggest a more simple implementation of the function as follows:
const openAppStore = async () =>
Linking.openURL(storeUrl).catch(() => {
openWebUrl(webStoreURL, () => setError(true));
});you can find openWebUrl implementation here
There was a problem hiding this comment.
Yes, setters are guaranteed to be always the same, but shouldn't useCallback prevent the function from being recreated? setError is related to the component so I think it should be just removed from the dep. list
ts/screens/modal/UpdateAppModal.tsx
Outdated
| <Text style={styles.text}>{I18n.t("messageUpdateApp")}</Text> | ||
| <Image | ||
| style={styles.img} | ||
| source={require("../../../img/icons/update-icon.png")} |
There was a problem hiding this comment.
In order to use the same pattern of all the codebase move this to the top of the component and use the import pattern:
import imageName from "../../../img/icons/update-icon.png"
List of changes proposed in this pull request
setStateas a sync operationHow to test
src/payloads/backend.tsin the dev server with:Linking.openURL(variable)calls toLinking.openURL("dummy")lets us fake failure casesBehavior
openURL()call fails, but the second call somehow triggers the Play Store app to open (?). I did confirm the first call fails by replacingwebStoreURL.androidwithhttps://www.google.itand it did open the Google homepage. Good for us. My guess is this is env-related.