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

Fix horizontal line visibility on login page when no SSO providers are available #9139

Conversation

muraliSingh7
Copy link
Contributor

This fixes issue #9094

All components ,, have their own Horizontal Separator so we don't require extra Horizontal Separator.

Please review @BOHEUS . Let me know if further adjustments are needed.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR removes redundant horizontal separators from the login page UI by eliminating explicit separator components in SignInUpWorkspaceScopeForm.tsx.

  • Removed unnecessary HorizontalSeparator from /packages/twenty-front/src/modules/auth/sign-in-up/components/SignInUpWorkspaceScopeForm.tsx since each auth provider (Google, Microsoft, SSO) already includes its own
  • Fixed visual issue where horizontal lines would appear even when no SSO providers were available
  • Improves UI consistency by only showing separators when auth providers are actually present

💡 (5/5) You can turn off certain types of comments like style here!

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -32,14 +32,7 @@ export const SignInUpWorkspaceScopeForm = () => {
{workspaceAuthProviders.microsoft && <SignInUpWithMicrosoft />}

{workspaceAuthProviders.sso.length > 0 && <SignInUpWithSSO />}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: inconsistent spacing - extra blank line after SSO but not after other providers

Copy link
Contributor

@AMoreaux AMoreaux left a comment

Choose a reason for hiding this comment

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

Hi @muraliSingh7 thanks for the contribution.

We want a visible separator between sso auth button and password auth button. So this line should be visible only if there are other methods to login. If only password is available to signin the line must be hide.

If you remove the code we will never see the separator.

We should check why this condition return false:

(workspaceAuthProviders.google ||
      workspaceAuthProviders.microsoft ||
      workspaceAuthProviders.sso.length > 0)

@muraliSingh7
Copy link
Contributor Author

muraliSingh7 commented Dec 19, 2024

The <SignInUpWithGoogle />, <SignInUpWithMicrosoft />, and <SignInUpWithSSO /> components each have their own horizontal separators, which shouldn't be visible. I mistakenly thought that each component had its own separator, which is why I was wondering why another separator is being shown.

Unfortunately, I can't investigate this locally, so I believe it may need to be handled by internal contributors.

@AMoreaux
Copy link
Contributor

I close it in favor of #9160

The <SignInUpWithGoogle />, <SignInUpWithMicrosoft />, and <SignInUpWithSSO /> components each have their own horizontal separators, which shouldn't be visible. I mistakenly thought that each component had its own separator, which is why I was wondering why another separator is being shown.

Unfortunately, I can't investigate this locally, so I believe it may need to be handled by internal contributors.

I understand the misunderstanding. The separator in each auth component is not visible it's for the space.

I opened this PR for the fix. Thanks a lot for the contribution.

@AMoreaux AMoreaux closed this Dec 20, 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