-
Notifications
You must be signed in to change notification settings - Fork 21
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
Notify Administrators when there is a new version of Praise out #522
Conversation
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.
Nice! Code works after adding REACT_APP
to the env variables.
I proposed some structural changes and will make a wireframe for the message design.
packages/frontend/.env.template
Outdated
REACT_APP_VERSION=$npm_package_version | ||
|
||
# Github repo info | ||
GITHUB_REPO_OWNER=commons-stack |
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.
These need to begin with REACT_APP
.
import { Nav } from '@/navigation/Nav'; | ||
import { AuthenticatedRoutes } from '@/navigation/AuthenticatedRoutes'; | ||
|
||
export const AuthenticatedLayout = (): JSX.Element | null => { | ||
const [sidebarOpen, setSidebarOpen] = useState(false); | ||
const siteNameSetting = useRecoilValue(SingleSetting('NAME')); | ||
const activeUserRoles = useRecoilValue(ActiveUserRoles); | ||
const isAdmin = useRecoilValue(HasRole(ROLE_ADMIN)); | ||
|
||
const githubVersion = useGithubVersionQuery().substring(1); |
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.
Consider moving the version handling logic to the hook as well to make the setup more clear and clean. Hook could then be named usePraiseAppVersion
and return an object in the format:
{
current: "v0.9.0",
latest: "v0.10.0",
newVersionAvailable: true
}
|
||
const githubVersion = useGithubVersionQuery().substring(1); | ||
const currentVersion = process.env.REACT_APP_VERSION; | ||
const isNewVersion = githubVersion !== currentVersion; |
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.
New version is available when githubVersion > currentVersion
.
packages/frontend/src/model/api.ts
Outdated
/** | ||
* External GET request | ||
*/ | ||
export const ExternalGet = selectorFamily< |
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.
This code is not related to the api. Move to separate file.
@@ -151,3 +151,28 @@ export const useSetSetting = (): useSetSettingReturn => { | |||
|
|||
return { setSetting }; | |||
}; | |||
|
|||
export const GithubVersionQuery = selector({ |
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.
This code is not related to settings. Move to separate file.
}, | ||
}); | ||
|
||
interface IGithubResponse { |
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 are not using a leading I
in interface naming.
packages/frontend/src/utils/api.ts
Outdated
* Client for external requests. | ||
* @returns | ||
*/ | ||
export const makeClient = (): AxiosInstance => { |
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.
This code is not related to the api. Move to separate file.
Here is a wireframe for the message layout: Release notes links to the GitHub release notes page. For example: https://github.com/commons-stack/praise/releases/tag/v0.10.0 |
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.
Nice!
I made some small changes:
- Moved things not related to either the api or GitHub/version numbers to the file
axios.ts
. We might use those functions for other purposes later on. - Deleted
useGithubVersionQuery
since it is basically only passing on the response. NowusePraiseAppVersion
callsuseRecoilValue(GithubVersionQuery)
directly - Added check
isResponseOk(response)
- Simplified styles of sticky message at top of screen. No need for
flex
etc since content is a simple text paragraph - Added the current version number as a fixed bottom right on all screens.
Resolves #493