Skip to content
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

chore: updates for login_app type [LIBS-405] #788

Closed
wants to merge 5 commits into from

Conversation

tomzemp
Copy link
Member

@tomzemp tomzemp commented Feb 21, 2023

This PR has changes necessary to build "public" login apps. A login app needs to not include the header bar (😅) but also needs to not rely on any requests to endpoints beyond the auth wall: hence the updates. See ticket https://dhis2.atlassian.net/jira/software/c/projects/LIBS/issues/LIBS-405

Note: regardless of whether we want to facilitate the building of custom login apps, we will need to implement most points of this PR. The last point (passing information about the app being of login_app type to app-runtime) is only necessary for facility hook in app-runtime (for building custom login apps)

changes

Header Bar

removed

UI Language

We will need to be able to update the UI language within our login app, so we could choose to not set the i18n language within app-platform for our login app, but I think it makes sense to do this generally (e.g. for creating general login apps) (uiLocale will be available publicly (DHIS2-14681) and the struts login app effectively defaults to this, but we could also use the browser language as the default language)

API Version

I think we will want the api version to be accessible to the app (e.g. for feature toggling)? It's also used in the app-runtime/services/data (e.g. https://github.com/dhis2/app-runtime/blob/master/services/data/src/links/RestAPILink.ts#L21). Currently, I don't think we have planned for this being accessible publicly?

Login Modal/Base URL

I'm not sure if we want to provide something similar to the LoginModal so that users can specify a base url? Otherwise, we could just have users

Offline capabilities

As discussed in our FE call (21 Feb 2023), we don't need offline support for login_apps, so I've removed the PWALoadingBoundary and OfflineInterfaceProvider from app-adapter, but I'm not sure if it's sufficient to just pass a null value for offlineInterface down to app-runtime Provider (https://github.com/dhis2/app-runtime/blob/master/runtime/src/Provider.tsx#L21) or if we need to adapt app-runtime as well?

Communicating type to app-runtime

This change is only applicable if we want to facilitate building custom login apps. For more information on facilitating custom login apps, see dhis2/ui#1395.

Whether app is of login_app type is sent to provider/app-runtime (f6abdd0#diff-113d225207bf5868bc80a76be8f1d7bd9eeaa06c184a8605ffb80bb78cf16e64R81) in order to facilitate the useLoginSettings hook (e.g. to populate this hook only for login apps).

For how this would work in app-runtime, see dhis2/app-runtime#1352

@tomzemp tomzemp marked this pull request as draft February 21, 2023 12:42
@tomzemp tomzemp changed the base branch from master to alpha August 16, 2023 08:35
@tomzemp tomzemp changed the title chore: example updates for login_app type chore: updates for login_app type [LIBS-405] Aug 22, 2023
@tomzemp tomzemp changed the base branch from alpha to beta August 22, 2023 12:39
@tomzemp tomzemp mentioned this pull request Aug 22, 2023
@tomzemp tomzemp changed the base branch from beta to master August 25, 2023 11:20
@tomzemp tomzemp changed the base branch from master to beta August 25, 2023 11:20
@@ -67,7 +68,7 @@ const compile = async ({
mode = 'development',
watch = false,
}) => {
const isApp = config.type === 'app'
const isApp = appTypes.includes(config.type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could export a convenience method .. isApp instead of repeating the .includes() around

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines +33 to +35
if (loginApp) {
const fakeSystemInfo = { version: '2.40-SNAPSHOT' }
setState({ loading: false, systemInfo: fakeSystemInfo })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can return at the end of the if (loginApp) branch .. to avoid the else and nesting for the rest, and keep it similar to other src/index.js

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -34,3 +37,28 @@ AppWrapper.propTypes = {
children: PropTypes.node,
plugin: PropTypes.bool,
}

export const LoginAppWrapper = ({ children }) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can move LoginAppWrapper into its own directory with supporting hooks, to keep the Login bits contained .. and avoid a lot of branching other than at the top level

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put it in a separate file. I wasn't sure about putting it in a different directory as it uses a number of the same components (in the same directory) as AppWrapper

@kabaros kabaros self-requested a review February 14, 2024 09:01
@tomzemp
Copy link
Member Author

tomzemp commented Feb 16, 2024

Closed and replaced by #831. This original PR is against beta and has some functionality that is no longer relevant as we have decided against facilitating custom login apps (and hence do not need to build a useLoginConfig hook into app-runtime)

@tomzemp tomzemp closed this Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants