This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
fix: correctly select and use user locale and application title text [DHIS2-8525] [DHIS2-8723] #399
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
amcgee
requested review from
janhenrikoverland,
jenniferarnesen and
martinkrulltott
Mohammer5
approved these changes
May 15, 2020
src/HeaderBar/Apps.js
Outdated
| @@ -66,7 +69,7 @@ function Search({ value, onChange, onIconClick, contextPath }) { | |||
| </span> | |||
|
|
|||
| <span> | |||
| <a href={`${contextPath}/dhis-web-menu-management`}> | |||
| <a href={`${baseUrl}dhis-web-menu-management`}> | |||
There was a problem hiding this comment.
I would prefer to copy the joinPath helper for now (until exposed by app-runtime).
This seems too brittle and could break instances.
| const baseUrl = useConfig().baseUrl | ||
| return ( | ||
| <a | ||
| href={`${baseUrl}/dhis-web-user-profile/#/profile`} |
There was a problem hiding this comment.
Contrary to the Apps.js, this one adds a slash. I think I've seen it in other files also.
We could just copye the joinPath as mentioned above
There was a problem hiding this comment.
Good catch, this is incorrect
Comment on lines
1
to
3
| import '../common/index.js' | ||
| import { Then } from 'cypress-cucumber-preprocessor/steps' | ||
| import { baseUrl } from '../common/index.js' |
There was a problem hiding this comment.
I guess the import in line 3 should be enough and we can remove the one in line 1
github-actions bot
pushed a commit
that referenced
this pull request
May 15, 2020
## [2.1.1-alpha.1](v2.1.0...v2.1.1-alpha.1) (2020-05-15) ### Bug Fixes * correctly select and use user locale and application title text [DHIS2-8525] [DHIS2-8723] ([#399](#399)) ([a7cb026](a7cb026))
|
🎉 This PR is included in version 2.1.1-alpha.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Merged
amcgee
added a commit
that referenced
this pull request
May 18, 2020
…[DHIS2-8525] [DHIS2-8723] (#399)
amcgee
pushed a commit
that referenced
this pull request
May 18, 2020
## [2.1.1-alpha.1](v2.1.0...v2.1.1-alpha.1) (2020-05-15) ### Bug Fixes * correctly select and use user locale and application title text [DHIS2-8525] [DHIS2-8723] ([#399](#399)) ([a7cb026](a7cb026))
amcgee
added a commit
that referenced
this pull request
May 18, 2020
…[DHIS2-8525] [DHIS2-8723] (#399)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This does a few things, all related to the two localization issues with the headerbar (DHIS2-8525 and DHIS2-8723
i18n.tcalls so that they are called at render time, not at module initializationsystemInfosince it loads a bunch of security-sensitive information. It was only used for (a)contextPath(which we can retrieve statically from the config context in app-runtime) and (b)systemNamewhich is untranslated and replaced withapplicationTitlebelowsystemSettings/applicationTitlewhich is the user-localized title for display inheaderBaruseEffecttouseMemo, to avoid needing a duplicate render (useEffect runs after first paint) and to avoid mutating the API response data (it should be treated as immutable)contextPaththroughout all components, instead loading the baseUrl from the config context in app-runtime wherever the baseUrl is actually needed. Note that this is slightly brittle as-written since it makes the assumption thatbaseUrlends in a/character. We should probably change this tojoinPath(baseUrl, relativePath)(a utility function we have in app-runtime but don't expose)default, even if we end up with a different singleton than the app uses (app-shell does this, so it should be a no-op in that environment).I'd like to merge this to the
alphabranch if it looks decent for testing in several apps, to avoid possible local dependency link issues.