Skip to content

Account height #2312

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 30, 2023
Merged

Account height #2312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
60a9b9c
Fixed the height and responsive layout of account settings
sdivyanshu90 Jul 18, 2023
83ec75f
Commented out unnecessary lines
sdivyanshu90 Jul 18, 2023
1a55892
Merge branch 'develop' into account_height
sdivyanshu90 Jul 18, 2023
2e28b01
Changes made according to the review
sdivyanshu90 Jul 20, 2023
d40b776
Merge branch 'develop' into account_height
lindapaiste Jul 30, 2023
eba7e34
Second review
sdivyanshu90 Jul 30, 2023
4f990bf
Merge branch 'develop' into account_height
lindapaiste Jul 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/styles/components/_account.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
color: getThemifyVariable('primary-text-color');
background-color: getThemifyVariable('background-color');
}
height: 100%;
min-height: 100%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect ⭐

}

.account-settings {
Expand Down
4 changes: 2 additions & 2 deletions client/styles/components/_forms.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@
}

.form__input {
width: 100%;
min-width: #{360 / $base-font-size}rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I gave you bad instructions here because I didn't realize that deleting this line will cause the inputs to be too narrow on large screens. 🤦‍♀️

Thinking this through...
Right now it is 360px wide always. This is a problem on small screens where 360px is too big. We want it to be 360px when possible but not too large to fit. The width: 100% alone doesn't do this because there is no width on the parent container (it will be too small on large screens). Doing a fixed width with max-width: 100% also doesn't work unless we also put a max-width on the container (it will overflow on small screens).

Option A - We put it back as it was before and merge this PR which fixes the background color issue.

Option B - The not perfect but very good fix.

.form__input {
  width: #{360 / $base-font-size}rem;
  max-width: 90vw;

The "perfect" fix would involve a max-width: 100%;, but that involves changing the CSS on a few ancestors and it becomes beyond the scope of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, thank you for the review. Right now, I will work on option B.

The "perfect" fix would involve a max-width: 100%;, but that involves changing the CSS on a few ancestors and it becomes beyond the scope of this PR.

Sure, I will work on the perfect fix, but not immediately. I will address it later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Re: "perfect" fix, I actually have a CSS which I think is good but this didn't seem like the right PR for it.

.form__input {
  width: #{360 / $base-font-size}rem;
  max-width: 100%;

.form-container {
  max-width: 100vw;
  padding: #{20 / $base-font-size}rem;

.form-container__content {
  max-width: 100%;
  & > * {
    max-width: 100%;
  }

max-width: 90vw;
width: #{360 / $base-font-size}rem;
height: #{40 / $base-font-size}rem;
font-size: #{16 / $base-font-size}rem;
@include themify() {
Expand Down