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

feat(react): add loading states, update SignIn and sample app, Add rollup for react. #15

Merged
merged 33 commits into from
May 21, 2024

Conversation

movinsilva
Copy link
Contributor

Purpose

  • Add loading states for components
  • Update SignIn component and its fragments.
  • Introduce new loading states for the asgardeo provider
  • Add rollup for react package
  • Update sample app

Related Issues

Related PRs

  • N/A

Checklist

  • e2e cypress tests locally verified.
  • Manual test round performed and verified.
  • UX/UI review done on the final implementation.
  • Documentation provided. (Add links if there are any)
  • Unit tests provided. (Add links if there are any)
  • Integration tests provided. (Add links if there are any)

Security checks

Comment on lines +50 to +51
enableConsoleBranding: authClientConfig?.enableConsoleBranding ?? true,
enableConsoleTextBranding: authClientConfig?.enableConsoleTextBranding ?? true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the console part here? How about enableBranding and enableTextBranding? @brionmario @dasuni-30 WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Branding is taken from props as well.
Need to specify this is coming from asgardeo/IS, right?

external: ['react', 'react-dom'],
input: 'src/index.ts',
onwarn(warning, warn) {
// Suppress this error message... there are hundreds of them
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to supress this error?

@@ -113,18 +115,22 @@ const SignIn: FC<SignInProps> = (props: SignInProps) => {
throw new AsgardeoUIException('REACT_UI-SIGN_IN-HA-IV02', 'Auth response is undefined.');
}

setIsComponentLoading(true);
authContext.setIsAuthLoading(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to expose it directly from the context rather than using authContext.setIsAuthLoading

Comment on lines +48 to +52
@keyframes fade-in {
to {
opacity: 1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to add animations IMO as this could clash with the application's aesthetic. That should be on the hands of the app developer

Comment on lines +44 to +48
@keyframes fade-in {
to {
opacity: 1;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above. Check in other places


const [modalVisible, setModalVisible] = useState<boolean>(false);

const authContext: AuthContext | undefined = useContext(AsgardeoContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of directly referring context, can't we use a custom hook to get these values? it looks more readable and elegant

@@ -37,7 +37,9 @@ const useAuthentication = (): UseAuthentication => {
const signOut: () => void = () => {
signOutApiCall().then(() => {
sessionStorage.clear();
window.location.reload();
if (contextValue.onSignOutRef.current) {
contextValue.onSignOutRef.current();
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this?

recipes/react-vite/.env Outdated Show resolved Hide resolved
Copy link
Contributor

@DonOmalVindula DonOmalVindula left a comment

Choose a reason for hiding this comment

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

Approved. The changes requested must be addressed in a follow-up PR

@dasuni-30 dasuni-30 merged commit d26d511 into asgardeo:main May 21, 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.

3 participants