Skip to content

Fixed plan selector alignment and plan blocks size on tablet screen sizes #101994

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 28, 2025
Merged

Fixed plan selector alignment and plan blocks size on tablet screen sizes #101994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions client/landing/stepper/declarative-flow/internals/steps-repository/unified-plans/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,22 @@
}
}

.step-container-v2--plans {
.plans-wrapper {
margin: 0;
padding: 0;
.step-container-v2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we need this change? Specificity? Can we do it without? I don't like the idea of targeting step-container-v2 directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. The .step-container-v2--plans selector won't allow us to update the subtitle, so for that we need to start on a parent element. That's why I used the .step-container-v2.

image

We could implement it outside of the current CSS, something like this:

image

But it feels more organized to keep everything inside the same parent, since all these changes are specific for the new step container .step-container-v2.

I don't like the idea of targeting step-container-v2 directly.

Interesting, can you elaborate more on why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @zaguiini concern is that by making the inner contents tightly coupled to the step container can lead to unintended style inheritance in other components of the step container and cause unintended side-effects.
I think having outside as it's own class is the safer approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well this is fine. It's going to be reworked anyway. Let's move forward.

.step-container-v2--plans {
.plans-wrapper {
margin: 0;
padding: 0;
}
}

// This is temporary until we make the plans step more consistent according to the designs.
@media(max-width: calc($break-small - 1px)) {
.plans-features-main .plans-features-main__subheader {
text-align: left;
}
}

@media(max-width: calc($break-medium - 1px)) {
.plan-type-selector {
width: 100%;
}
Expand Down
4 changes: 2 additions & 2 deletions client/my-sites/plans-features-main/components/plans-page-subheader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ const PlansPageSubheader = ( {
return (
<>
{ createWithBigSky && (
<Subheader>
<Subheader className="plans-features-main__subheader">
{ translate(
'Build your site quickly with our AI Website Builder or {{link}}start with a free plan{{/link}}.',
{
Expand All @@ -162,7 +162,7 @@ const PlansPageSubheader = ( {
</Subheader>
) }
{ ! createWithBigSky && deemphasizeFreePlan && offeringFreePlan ? (
<Subheader>
<Subheader className="plans-features-main__subheader">
{ translate(
'Unlock a powerful bundle of features. Or {{link}}start with a free plan{{/link}}.',
{
Expand Down
2 changes: 1 addition & 1 deletion packages/plans-grid-next/src/components/features-grid/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ const WrappedFeaturesGrid = ( props: FeaturesGridExternalProps ) => {
}

// we want to fit 3 plans per row in this breakpoint
const mediumBreakpoint = isInSiteDashboard ? 667 : 741;
const mediumBreakpoint = 669;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason behind this change? If we want to customize this further, then we'll likely need a prop, otherwise it will break other pages, no?

Copy link
Contributor Author

@paulopmt1 paulopmt1 Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What's the reason behind this change?

We want to match the breakpoint with the max-width break-small defined here.
So that will generate this behavior:

Screen.Recording.2025-03-27.at.16.52.07.mov

Without setting it, we'll have this weird behavior where a single plan is shown after the break-small:

Screen.Recording.2025-03-27.at.16.53.31.mov

If we want to customize this further, then we'll likely need a prop

The original value was introduced here.

I don't think we want to keep customizing it since it's hard-coded to this screen. @fushar can you confirm that?

otherwise it will break other pages, no?

I tested that feature locally (which has the flag isInSiteDashboard true) to ensure the value chance of 2px (before was 667) wouldn't change their behavior, and it looks good.

I also tested other flows where we show the plans screen, like /plans, and the new change (669 instead 741) looks good as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks good on my end, too! Yeah, it would be good to have a single value here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 👍🏻


return new Map< GridSize, number >( [
[ 'small', 0 ],
Expand Down
Loading