refactor(app): trigger UpdateAppModal based on state.alerts #6708
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
mcous
requested review from
a team,
b-cooper,
Kadee80 and
shlokamin
and removed request for
a team
mcous
added
robot-svcs
mcous
changed the title
sfoster1
approved these changes
Oct 9, 2020
6 tasks
mcous
commented
Oct 9, 2020
| <a className={styles.sync_link} onClick={props.onClick} disabled> | ||
| click here | ||
| </a> | ||
| <Link href="#" color={C_BLUE} onClick={props.onClick}> |
There was a problem hiding this comment.
Note: <a href="#" ... misbehaves, in that it will trigger react-router routing. I've replaced this with a more semantically correct <button> in #6715 that is styled like a link
| @@ -51,7 +56,9 @@ export function AdvancedSettingsCard(props: Props): React.Node { | |||
| const handleChannel = event => | |||
| dispatch(Config.updateConfigValue('update.channel', event.target.value)) | |||
|
|
|||
| React.useEffect(props.checkUpdate, [channel]) | |||
There was a problem hiding this comment.
This logic moved to an epic
| @@ -51,7 +56,9 @@ export function AdvancedSettingsCard(props: Props): React.Node { | |||
| const handleChannel = event => | |||
| dispatch(Config.updateConfigValue('update.channel', event.target.value)) | |||
|
|
|||
| React.useEffect(props.checkUpdate, [channel]) | |||
| useMountEffect(() => { | |||
| dispatch(checkShellUpdate()) | |||
There was a problem hiding this comment.
Wait actually this whole thing can go away; a sibling component is already doing this. Will address in #6715
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.
Overview
This PR switches the app update notification from a custom system to the more generic
app/src/alertssystem. This work unblocks #6684, and also means that when the<UpdateAppModal>pops up, it doesn't shunt you automatically to a different part of the app; it just opens up over whatever page you're already on.Old and busted:
<UpdateAppModal>is displayed based on navigating to the/more/app/updateroutestate.shell.update.availableandstate.shell.update.seenand return a react-router<Redirect>if the notification needs to be seen/more/apppage), a react-router<Link>points to `/more/app/updateNew hotness:
state.shell.updateand if it sees an update become available, it sends aalerts:ALERT_TRIGGEREDaction withalertId: 'appUpdateAvailable'in the payload<Alerts>component renders an<UpdateAppModal>component if theappUpdateAvailablealert is active<UpdateAppModal>in a<Portal>Changelog
useSelectordoesn't change and the store never emits a change notification,useSelectorwill always return the cached valuestore.getState.mockReturnValueReview requests
To test this on a dev app, mess with the
versioninappandapp-shell's package.json so the app thinks it's behind 3.21.0. The following logic was touched:Risk assessment
This is an important part of the UI, so medium risk here. Everything looks good in my click around smoke testing, all new components were TDD'd, and I added some tests to existing (testless) components to ensure new functionality around modal display was covered.