-
Notifications
You must be signed in to change notification settings - Fork 153
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
Yt regrets landing top half #7842
Conversation
…n.mozilla.org into yt-extension-page-base
…n.mozilla.org into yt-extension-page-base
This PR introduces visual differences. Click here to inspect the diffs. |
Hi @beccaklam! Wanted to give a heads up that at the moment I am using a placeholder youtube video. I was wondering if you could send me the link to the updated video, and I can update this PR to show the correct one. Thanks in advance! Also wanted to note that the browser sensing email button will be added in a later PR |
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 @danielfmiranda this is looking so good! 💯
Just some small things:
- On Firefox on my laptop, I'm not getting the "content peek" so was just wondering if the 80% view port styling was added. I get a little peek in Chrome but it doesn't look like 20%?
- On screen sizes around ~800px in width I did notice it looks a little odd because a big chunk of space at the top. Would it be possible to remove that (the black space not in purple)?
Thank you for the feedback @beccaklam! I have added your suggested changes and re-requested review |
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 @danielfmiranda thanks for making those changes! The 'content-peek' is now working perfectly for me. Unfortunately I think removing the padding on screen widths around ~800px may have broken something?
Also, just noticed this, but would it be possible to have the two buttons be on the same line instead of stacked (I get this view when I rotate mobile view to horizontal)? Maybe the breakpoint for stacking could start around 500px (or whatever you think is best)?
Hi @beccaklam! No problem I have now added a minimum height to the header section, which resolves the issue of the buttons cutting into the bottom half text section: I also updated the buttons to be in-line throughout the small breakpoint since there is room for them: However, I did want to mention that they still stack on the medium breakpoint since they don't fit side-by-side unless we reduce font/button/icon size for that particular breakpoint. If you would like me to do that instead, please let me know! Thank you 😄 |
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 @danielfmiranda Thanks so much for those changes! It's pretty much 99% there I just noticed that when I check the screen at 568x320 (iPhone SE horizontal viewport) the buttons are still stacked. Would it be an easy fix to also make those buttons inline at that screen width too?
Oh and the buttons stacking on the medium breakpoint is fine :) I had it styled that way in the designs anyway! |
…dation.mozilla.org into yt-regrets-landing-top-half
Hi @beccaklam! No problem, I have now updated the buttons to sit side by side if room allows for it, if not, they then stack on top of one another. So I am thinking that this should resolve the issue that you mentioned above. The change is now live and ready for re-review, thanks! |
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.
On the tailwind config:
- change the new 100 colors to light instead to be consistent with the other colors
- opacity: {
40: 0.4,
} <--- Remove this since this already exists as a default value
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.
Thank you for making that change @danielfmiranda ! This looks good to merge from a design stand point! 👍
Closes #7793
Related PRs/issues #
Link to sample test page: https://foundation-s-yt-regrets-60mtiy.herokuapp.com/en/youtube/regretsreporter/
Steps to test: