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

Streamline fx mobile grid with one column hidden #15653

Merged
merged 3 commits into from
Dec 5, 2024

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Dec 3, 2024

One-line summary

When on .ios or .android the send-to-device form is hidden, making the 2col split with just empty space in 50% of the screen feel like a bug. This overrides the split to a single column to make better use of the space (esp. on mobile).

Significant changes and points to review

For cases when the form is hidden (making the second column empty), this additionally:

  1. resets the grid/display for MD+ breakpoints
  2. and also constraints the width in LG+ to be able to keep the noodle graphics but not overflow into it (too much).

Issue / Bugzilla link

Resolves #15648

Testing

http://localhost:8000/hi-IN/firefox/browsers/mobile/focus/
http://localhost:8000/ar/firefox/browsers/mobile/android/
http://localhost:8000/el/firefox/browsers/mobile/ios/

(compare desktop vs. mobile device or spoof the uastring to include "android" and "mobile", portrait vs. landscape mode and several breakpoints…)


After: × Before:

Screenshot 2024-12-04 at 1 28 16 Screenshot 2024-12-04 at 1 33 52 Screenshot 2024-12-05 at 13 45 19

@janbrasna janbrasna requested a review from a team as a code owner December 3, 2024 23:45
@maureenlholland maureenlholland added Needs Review Awaiting code review Frontend HTML, CSS, JS... client side stuff Review: XS Code review time: up to 30min Review: µ Code review time: 5 minutes or less and removed Review: XS Code review time: up to 30min labels Dec 4, 2024
@maureenlholland
Copy link
Collaborator

might have overlapping changes with another PR: #15648 (comment)

@janbrasna
Copy link
Contributor Author

Yup I'm watching Craig's #15654 — these solve two isolated issues and may complement each other, and should not interfere in any way or conflict code-wise.

(This one is addressing just the empty space as seen above in the screen shots, the other PR changes text size to be closer to the original, as that can also be seen in the images above demonstrating the heading difference between m24 and prod as well.)

@maureenlholland maureenlholland self-assigned this Dec 4, 2024
@maureenlholland maureenlholland removed the Needs Review Awaiting code review label Dec 4, 2024
Copy link
Collaborator

@maureenlholland maureenlholland left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Looks good in browserstack testing and some spoofed dev tools testing 🖼️

r+wc

// and avoid the noodles background where the form was
@media #{$mq-lg} {
.c-page-header .mzp-c-split-body {
width: 65%;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, non-blocking: we might want to be a bit more conservative with avoiding the noodle background where "focus" pages are concerned because it's a darker background color. This was coming up for me on some landscape testing on tablets (around 1080px), minor edge cases

body-overlap

Copy link
Collaborator

@maureenlholland maureenlholland Dec 4, 2024

Choose a reason for hiding this comment

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

hang on, maybe this was an issue with browserstack, I'm not sure that background is supposed to be there given your screenshots

edit: looks like it will be there on above large breakpoint screens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it is supposed to be there in the appropriate viewport.

It's sort of intentional — but you're absolutely right, given the angle in the graphics — once the heading gets smaller in the adjacent PR, the body copy will move upwards — with greater chance actually interfering with the noodle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually considering removing the noodle graphics completely, if on mobile, thus in one column generally, yet on wide viewport. But that felt like too much design decisions to consider so I kinda cheated using 100% width only when there's no background, and bumped the 50% split to 65% when the background is present, to give the content a bit more space.

So this can:

  • either wait for Craig's type changes to land, and update from that to see the impact,
  • remove the part trying to cheat around the noodle background presence and keep it at 50%,
  • or remove the noodle on mobile completely, no matter what screen size, and keep that in one column still,
  • or get creative around applying css tricks/filters to the noodle to make it e.g. lighter(?) just for this specific case to be on the safe side,
  • or add a solid white background to the paragraph text in case it runs into the image to keep legibility in worst case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

either wait for Craig's type changes to land, and update from that to see the impact,
remove the part trying to cheat around the noodle background presence and keep it at 50%,

these options sound best to me

if the 50% doesn't seem like a generous enough line length on large screens, I would say the background removal is next best option (it's not really serving the purpose of highlighting a call to action anymore and readability takes priority)

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no reason to cut this close. Give those noodles a real boot out of the way 🙂

Copy link
Contributor Author

@janbrasna janbrasna Dec 5, 2024

Choose a reason for hiding this comment

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

I think I'm happy now, only keeping ~half of every illustration with some low effort styling, giving more space to the content. Updated the description with the current mobile+landscape output and below are some edgiest edge cases that pretty much should not happen in standard resolutions, but wanted to cover the worst cases here too (dimensions included in the sshots).

Let's see if that's viable (and the few lines of code don't feel too expensive to maintain for such a minor use case) or it warrants a more violent approach O:) — thanks for the feedback!

Screenshot 2024-12-05 at 11 33 16
Screenshot 2024-12-05 at 11 31 43
Screenshot 2024-12-05 at 11 30 22
Screenshot 2024-12-05 at 11 28 52

Of course, this is only a negligible subset of the overall change, so no need to spend too much time on in, it's easy to edit out…

@stephaniehobson stephaniehobson merged commit 62fc67e into mozilla:main Dec 5, 2024
5 checks passed
@janbrasna janbrasna deleted the upd/fx-mobile-grid branch December 5, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend HTML, CSS, JS... client side stuff Review: µ Code review time: 5 minutes or less
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heading Text Width on Product Pages
3 participants