-
Notifications
You must be signed in to change notification settings - Fork 6
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(core): add logout, getBrandingCSS & refactor branding, profile files #9
Conversation
downgrade ts version to get the support for eslint
…ng-preference-text
return ''; | ||
} | ||
|
||
const footerFontColor: string = !isEmpty(theme[theme.activeTheme].footer.font.color) |
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.
Lets create a local variable for theme[theme.activeTheme] and reference that everywhere.
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 took this from a current implementation in identity apps. Its better to update.
Added with 97b0093
@@ -17,7 +17,7 @@ | |||
*/ | |||
|
|||
export * from './api/public-api'; | |||
export {default as getBranding} from './branding/branding'; | |||
export * from './branding/public'; |
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.
do we need to use index files?
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.
It's not strictly necessary to use an index.ts
file. But as the main entry point for the package, wouldn't it be good to follow the common convention?
This could improve the readability of the code as well.
packages/core/src/api/logout.ts
Outdated
* console.error('Failed to log out:', error); | ||
* }); | ||
*/ | ||
const logout = async (): Promise<void> => { |
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.
Should this be logout
or signOut
?
Are we using signIn
or Login
in the SDK?
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'll be having a component called SignIn
. (But this is not exposed from this js-ui-core
package.)
We don't have a separate function called signIn or login. Rather we handle this process using authorize
and authenticate
functions which are api calls to authorize
and authn
endpoints.
I went with the logout
name to match the endpoint, oidc/logout
and the IS & Asgardeo docs also follow the logout
name. But it seems the currently available js SDKs use the signOut
more.
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.
Updated to signOut
with 0792b1f
packages/core/src/utils/is-empty.ts
Outdated
* isEmpty([1, 2, 3]); // => false | ||
* isEmpty({ a: 1 }); // => false | ||
*/ | ||
const isEmpty = (value: unknown): boolean => { |
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.
Lets use https://www.npmjs.com/package/lodash.isempty instead.
Can send this in a followup PR if needed.
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.
Wouldn't that be good to reduce the dependencies?
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.
updated with 48dc84f
…isEmpty implementation
Purpose
Related Issues
Related PRs
Checklist
Security checks