-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(ui): release notes modal #1345
base: main
Are you sure you want to change the base?
Conversation
…into release-notes-modal
src/containers/floating/Settings/sections/releaseNotes/useReleaseNotes.ts
Outdated
Show resolved
Hide resolved
src/containers/phase/Setup/Setup.tsx
Outdated
|
||
export default function Setup() { | ||
useSetUp(); | ||
useReleaseNotes({ triggerEffect: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM & tested locally with the admin trigger
just a bit concerned about this one in during setup and how it will react with auto update stuff? 😅 might be good for @mmrrnn to have a look through too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shanimal08 good point, I can move the trigger for the modal to happen after setup completes. That way it wont conflict with the updater.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not to invoke check_for_updates
to ensure there is no update available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add it to the get_last_changelog_version
response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmrrnn I moved it to the MainView
so it shouldn't conflict with the Updater anymore.
One weird requirement for this is that CHANGELOG.md
might not be updated in time for a release. So this will pop only when the latestVersion
in CHANGELOG.md
( fetched from github main ) matches the appVersion
number, and when latestVersion
does not equal get_last_changelog_version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fine 👍
@brianp @shanimal08 I've updated the modal to trigger in I also updated the |
This adds a Release Notes modal that pops up after a new app version is detected.
Functionality requirements: