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

Overflow gets cancelled on login | BUG #184

Merged
merged 10 commits into from
Nov 26, 2023
Merged

Overflow gets cancelled on login | BUG #184

merged 10 commits into from
Nov 26, 2023

Conversation

achandrabalan
Copy link
Contributor

Implementation description

  • I've had this weird bug when I logout and log back in that basically freezes the screen and blocks scrolling on the actual page and only allows it on the table. I've been able to get past this with refreshing the page but that is not very intuitive for the user.

Steps to test

  1. See if you can reproduce the bug on your end, press log out and then sign back in. Does the scroll lock?
  2. If so, does this fix it for you?

What should reviewers focus on?

  • I think this fixes it on my side not sure if it's a universal fix tho
  • Also are there any risks to setting this here? I'm not too sure how it affects the rest of the app.

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR
  • If I have made API changes, I have updated the REST API Docs
  • IF I have made changes to the db/models, I have updated the Data Models Page
  • I have documented this PR in the Production Release Page
  • I have updated other Docs as needed

@github-actions
Copy link

github-actions bot commented Oct 20, 2023

Visit the preview URL for this PR (updated for commit 79d6559):

https://blueprintsupportivehousing--pr184-aathi-testing-c9ksqtqg.web.app

(expires Sun, 03 Dec 2023 20:38:35 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

Copy link
Contributor

@owen-sellner owen-sellner left a comment

Choose a reason for hiding this comment

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

Great job fixing the bug! I found an issue with horizontal overflow but the solution should be quick to implement.

frontend/src/components/auth/Credentials.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@owen-sellner owen-sellner left a comment

Choose a reason for hiding this comment

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

I noticed the same issue as before in Signup.tsx but after that I think this is ready.

frontend/src/components/auth/Signup.tsx Outdated Show resolved Hide resolved
@connor-bechthold connor-bechthold mentioned this pull request Nov 4, 2023
8 tasks
Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

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

So I noticed that the messages below the buttons for both the Signup and Login page are different - I suggested changes on the Signup Page that fix this that utilize Flexbox and removes the use of margin and padding (which could produce unexpected results on different screen sizes)

To keep things consistent the same changes can also be made on the Login page and then both forms should look identical styling wise and this should be good to go! Let me know if you have any questions/confusions with the suggestions at all.

frontend/src/components/forms/Signup.tsx Show resolved Hide resolved
frontend/src/components/forms/Signup.tsx Outdated Show resolved Hide resolved
frontend/src/components/forms/Signup.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

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

LOOKS GREAT I left like a million comments on this one so thanks for addressing everything 🙏

This should be good to merge once #202 is merged (just because it adds error messages on this page that will cause conflicts, but should be easier to address in this PR)

Copy link
Contributor

@owen-sellner owen-sellner left a comment

Choose a reason for hiding this comment

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

LGTM!!!!

@achandrabalan achandrabalan merged commit c0d7a1d into main Nov 26, 2023
3 checks passed
@achandrabalan achandrabalan deleted the aathi/testing branch November 26, 2023 20:58
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