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

[#12265] Modify minimum page height to factor in footer height #12406

Merged
merged 8 commits into from
Apr 27, 2023

Conversation

ryalin
Copy link
Contributor

@ryalin ryalin commented Apr 22, 2023

Fixes #12265

Outline of Solution

The issue was that as the widths of screen change, the footer dynamically stacks, increasing its height. In some cases where all the contents of the screen can fit, scrolling is still allowed because the min-height css parameter does not take the footer into account. I added some css properties to "/src/web/app/page.component.scss".

Steps I took:

  1. I determined which screen widths caused a change in footer height

  2. I used media queries to set the correct min-height based on screen width (which affects footer height)
    a) I went with this approach because it seemed the least convoluted

  3. I tested using the chrome web inspector to ensure everything worked for every screen size
    a) Screen sizes greater than 313 pixels- but no modern phone has a screen that small

@weiquu
Copy link
Contributor

weiquu commented Apr 23, 2023

Hi @destroyer806, for your solution, the screen widths are hardcoded into the .scss file. However, the hardcoded values seem to be dependent on the text of the footer itself. For example, if I remove the "Become a sponsor" text or change it to "Become a sponsor now!", the hardcoded values will have to be manually tested and changed.

I think the solution shouldn't be dependent on the text of the footer as it's 1. not very intuitive and 2. not flexible. Do let me know what you think.

@zhaojj2209
Copy link
Contributor

Hi @destroyer806, for your solution, the screen widths are hardcoded into the .scss file. However, the hardcoded values seem to be dependent on the text of the footer itself. For example, if I remove the "Become a sponsor" text or change it to "Become a sponsor now!", the hardcoded values will have to be manually tested and changed.

I think the solution shouldn't be dependent on the text of the footer as it's 1. not very intuitive and 2. not flexible. Do let me know what you think.

While hardcoding of values is generally frowned upon, handling of header/footer height is the one case where hardcoding is the only way to do it as far as I know. I don't think there is a good way to dynamically retrieve the height of the header/footer and factor it into the min width calculations. In fact, the TEAMMATES header height is already being hardcoded, though it's less complicated there since the header height stays the same at any screen width.

Thankfully, the footer content is rarely changed, so it is unlikely that the heights will need to be tweaked. The footer height is more likely to change on mobile, but scrolling is less of an issue there since there are fewer pages that are shorter than the mobile view height.

@weiquu
Copy link
Contributor

weiquu commented Apr 23, 2023

While hardcoding of values is generally frowned upon, handling of header/footer height is the one case where hardcoding is the only way to do it as far as I know. I don't think there is a good way to dynamically retrieve the height of the header/footer and factor it into the min width calculations. In fact, the TEAMMATES header height is already being hardcoded, though it's less complicated there since the header height stays the same at any screen width.

Thankfully, the footer content is rarely changed, so it is unlikely that the heights will need to be tweaked. The footer height is more likely to change on mobile, but scrolling is less of an issue there since there are fewer pages that are shorter than the mobile view height.

Fair enough! Thanks for the insight (: I was thinking of some way that involves getting the height of the footer (e.g. by clientHeight) after the page elements have been loaded and then setting the min-height attribute afterwards, but I'm not sure if that'll work.

@destroyer806 Sorry please ignore my above comment then! No need for changes on that end

src/web/app/page.component.scss Outdated Show resolved Hide resolved
src/web/app/page.component.scss Outdated Show resolved Hide resolved
@zhaojj2209 zhaojj2209 added the s.ToReview The PR is waiting for review(s) label Apr 23, 2023
@ryalin
Copy link
Contributor Author

ryalin commented Apr 23, 2023

I do agree that hardcoding values is less than ideal, but through (admittedly) hours of trying, changing height based on screen width seems to be the only way. Thanks to @weiquu for pointing that out

@ryalin
Copy link
Contributor Author

ryalin commented Apr 24, 2023

Hi! I made the changes requested and tested them out. Everything works as it should. Do I have to do anything else on my end before possible merging?

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@zhaojj2209 zhaojj2209 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 25, 2023
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM!

@weiquu weiquu added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 27, 2023
@weiquu weiquu merged commit a4d335d into TEAMMATES:master Apr 27, 2023
@samuelfangjw samuelfangjw added the c.Bug Bug/defect report label Jul 14, 2023
@samuelfangjw samuelfangjw added this to the V8.28.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify minimum page height to factor in footer height
4 participants