-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
change media query to desktop-up #3774
change media query to desktop-up #3774
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Availability: 6 - 11 pm 12/27, 10 am - 2 pm 12/28 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Jaretzbalba, the footer looks really nice and smooth as the screen size changes. Question: Was the footer supposed to be sticky? That was not clear to me from the Figma design. I noticed position: sticky on .main-footer, but I'm not sure if that is necessary and if so, it is not clear what effect that style is having.
@roslynwythe I saw that as well but left it since I was focusing more on the layout than the positioning of the footer itself. That sticky position is not doing anything from what I can tell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @Jaretzbalba ! Source and destination branches are correct, and after your change to the media query, the footer now looks good at all screen sizes. I am somewhat concerned because the CSS on the main-footer class seems to indicate the intention to make the footer sticky, but it does not behave in a "sticky" manner. However, this same situation exists in feature-homepage-launch and this issue mentioned nothing about the sticky behavior, so I assume the sticky situation will be handled in a separate issue.
Review ETA: EOD Jan 9, 2023 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work @Jaretzbalba ! 👍
Nice job finding the specific @media
query that needed to be changed.
I did notice your issue is currently missing ☑️ for most of the action items. I recommend that you click those checkboxes as you complete each task to help pull request reviewers.
Hey @roslynwythe if you are interested in pursuing your suggestion to re-examine the I tracked down the PR that changed the position to sticky but I don't have time to look into it right now: |
Thank you @MattPereira for finding that PR. Reading it, I learned that the
use of position: sticky was an intentional choice, not to fulfill a
requirement for a sticky footer, but to solve a challenging CSS positioning
problem. I'd never seen that usage before.
…On Mon, Jan 9, 2023 at 3:17 PM Matt Pereira ***@***.***> wrote:
Hey @roslynwythe <https://github.com/roslynwythe> if you are interested
in pursuing your suggestion to re-examine the position: sticky property
you always have the option to create an Emergent Request
<https://github.com/hackforla/website/issues/new?assignees=&labels=size%3A+0.25pt&template=emergent-request.md&title=ER%3A+%5Breplace+with+info+%5D+>
I tracked down the PR that changed the position to sticky but I don't have
time to look into it right now:
- #3183 <#3183>
—
Reply to this email directly, view it on GitHub
<#3774 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABIRM2JX7LBK2AJDOS6A5FLWRSMAVANCNFSM6AAAAAATKVSK44>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fixes #3437
What changes did you make and why did you make them ?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied