Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Set margin between "Forgot passwords?" and SSO buttons #8143

Closed
wants to merge 10 commits into from
Closed

Set margin between "Forgot passwords?" and SSO buttons #8143

wants to merge 10 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 24, 2022

Fixes element-hq/element-web#21545

Signed-off-by: Suguru Hirahara [email protected]

after.mp4

type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

Preview: https://pr8143--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@luixxiul luixxiul requested a review from a team as a code owner March 24, 2022 11:05
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Mar 24, 2022
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #8143 (d9907fe) into develop (61076c3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #8143   +/-   ##
========================================
  Coverage    29.25%   29.25%           
========================================
  Files          863      863           
  Lines        49876    49876           
  Branches     12696    12696           
========================================
  Hits         14592    14592           
  Misses       35284    35284           

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

hmm, maintaining either the button itself or moving the spinner above the SSO buttons would probably be best - both before and after this bug it's far too easy to double click and end up in a bad spot.

Passing off to design before code review.

@turt2live turt2live requested a review from a team April 4, 2022 06:24
@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 4, 2022

This case #8163 should just be reopened to fix the issue.

@janogarcia
Copy link
Contributor

Thanks @luixxiul, but I agree with @turt2live remarks. The correct way to handle it would be to use a busy state for the button (disabled button, loading spinner inside), so there are no UI reflows / layout shifts involved.

Two questions for @turt2live:

  • Do we have such a busy state already that we can reuse?
  • This shouldn't be on the contributor's plate if not interested / can't contribute it. In that case, should we just close this PR without merging?

@luixxiul
Copy link
Contributor Author

The correct way to handle it would be to use a busy state for the button (disabled button, loading spinner inside), so there are no UI reflows / layout shifts involved.

I would totally agree 👍

@luixxiul luixxiul closed this Apr 12, 2022
@luixxiul luixxiul deleted the Login branch April 12, 2022 11:39
@turt2live
Copy link
Member

  • Do we have such a busy state already that we can reuse?

We have a disabled state, yea. Should be easy to copy.

  • This shouldn't be on the contributor's plate if not interested / can't contribute it. In that case, should we just close this PR without merging?

Yup, though they've done so from the looks of it ;)

@luixxiul
Copy link
Contributor Author

luixxiul commented Apr 12, 2022

#8163 has been closed in favor of this. That should be reopened. It should realize element-hq/element-web#21545 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Margin between "Forgot password?" and SSO buttons
3 participants