Account height

[sdivyanshu90]

Fixes :
When the High Contrast theme is enabled, a white space or box appears at the bottom of the page. This issue is caused by the height of the element.

Screenshot of the error:

Changes:

• changes height to min-height

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[sdivyanshu90]

Hi @raclim, could you please review my pull request? Thank you!

[lindapaiste]

Thanks for finding this issue! I see that it happens when the screen height is smaller than the height of the content. The part that you have to scroll down to see doesn't get the dark background.

I do kinda wonder why we are putting the background color on the .account-settings__container div and not on a higher-up element like the body or div id="root" but that's beyond the scope of this PR.

As far as fixing the issue, your change of width to min-width is perfect! The other changes are a bit more suspect. I'll leave comments on specific lines in a sec.

[lindapaiste]

Perfect ⭐

[lindapaiste]

If the width is 100% then the min-width of 10% will never be used. The 10% will never be greater than the 100%. So this doesn't make a ton of sense. BUT....it actually is an improvement over what we have now! What you've discovered is that the current min-width: 360px (min-width: #{360 / $base-font-size}rem;) is not good as it causes horizontal overflow when the screen is narrower than ~390px.

Instead of changing it to min-width: 10%;, let's delete the existing line and replace it with nothing.

[lindapaiste]

I don't think this line would come into play it all. Correct me if I'm wrong. I think you should delete this.

[lindapaiste]

It's probably better to delete this than to add commented-out styles into the codebase.

[sdivyanshu90]

Thank you for your review, @lindapaiste! I've implemented all the mentioned changes.

[lindapaiste]

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.

[sdivyanshu90]

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.

[lindapaiste]

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%;
  }