-
Notifications
You must be signed in to change notification settings - Fork 23
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: DHIS2-10525 adds url #1520
Conversation
const { baseUrl } = config; | ||
const instanceBaseUrl = baseUrl.split('/api')[0]; | ||
window.location.href = `${instanceBaseUrl}/dhis-web-tracker-capture/#/dashboard?tei=${teiId}&ou=${orgUnitId}&${scopeSearchParam}`; | ||
const base64Url = btoa(`/capture-app/index.html#${currentUrl}`); |
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.
The urls will be of the following format /capture-app/index.html#${currentUrl}
which basically will be something like
/capture-app/index.html#/new?programId=ID&orgUnitId=ID
or
/capture-app/index.html#/search?trackedEntityType=ID&orgUnitId=ID
and so on
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.
I have a suggestion :) Can the returnUrl
be the whole url except the baseUrl
? That way we would avoid any potential problems if the app name (the app name representation in the url) changes (or if we're uploading a custom app).
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.
uhuh surely can do that
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.
Alternatively we need to change capture-app
in the url to dhis-web-capture
. But I would prefer the above.
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.
Help me understand this right, we want to omit the first bit of this
const base64Url = btoa(`/capture-app/index.html#${currentUrl}`);
and have as a result this ->
const base64Url = btoa(`/index.html#${currentUrl}`);
?
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.
Problem is I dont know bu heart what value the baseUrl
holds usually..
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.
On line 4 in the current file you have: const { baseUrl } = config;
The return url I would like to be the entire url in the address bar, except the baseUrl (from line 4).
Does that make sense?
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.
Almost. Can you give an example please?
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.
Okay now got it. Makes sense
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.
On second thought, it won't work as I imagined. Let's go with the easy solution.
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.
One suggestion, apart from that looks good 👍
const { baseUrl } = config; | ||
const instanceBaseUrl = baseUrl.split('/api')[0]; | ||
window.location.href = `${instanceBaseUrl}/dhis-web-tracker-capture/#/dashboard?tei=${teiId}&ou=${orgUnitId}&${scopeSearchParam}`; | ||
const base64Url = btoa(`/capture-app/index.html#${currentUrl}`); |
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.
I have a suggestion :) Can the returnUrl
be the whole url except the baseUrl
? That way we would avoid any potential problems if the app name (the app name representation in the url) changes (or if we're uploading a custom app).
const returnUrl = | ||
// when developing locally the following will always be false | ||
window.location.href.includes(instanceBaseUrl) | ||
? | ||
window.location.href.split(instanceBaseUrl)[1] | ||
: | ||
`/dhis-web-capture/#${currentUrl}`; |
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.
I dont think this is self explanatory but when you develop locally you can not match the instanceBaseUrl
(which will be somthing between these lineshttps://debug.dhis2.org/ca-2.36
) with the current window.location.href
(which is http://localhost:3000
or similar).
For this cases I hardcoded the value /dhis-web-capture/#${currentUrl}
I think there is much room for improvement here but cant find a better solution. Very open to suggestions since in my eyes this is overcomplicated now.
🎉 This PR is included in version 1.13.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Hey both
ticket is https://jira.dhis2.org/browse/DHIS2-10525
As we have discussed I encoded the url, let me know if you find that I missed anything.