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

C24_WMDE_Desktop_DE_22 #670

Merged
merged 4 commits into from
Dec 20, 2024
Merged

C24_WMDE_Desktop_DE_22 #670

merged 4 commits into from
Dec 20, 2024

Conversation

Sperling-0
Copy link
Contributor

Both banners:

  • The banners are based on the control banner of desktop-de-21.

Variant banner:

  • The variant banner shows an additional sentence.

Ticket: https://phabricator.wikimedia.org/T382301

@Sperling-0 Sperling-0 force-pushed the C24_WMDE_Desktop_DE_22 branch 2 times, most recently from 0816c5d to 9b2fa39 Compare December 17, 2024 14:49
@moiikana moiikana force-pushed the C24_WMDE_Desktop_DE_21 branch from 04f70d5 to 07a2777 Compare December 17, 2024 16:35
@Sperling-0 Sperling-0 force-pushed the C24_WMDE_Desktop_DE_22 branch from 9b2fa39 to 6505d7d Compare December 19, 2024 13:36
Base automatically changed from C24_WMDE_Desktop_DE_21 to main December 19, 2024 13:56
@@ -12,7 +12,7 @@
@use 'Banner';
@use 'src/themes/UseOfFunds/UseOfFunds';
@use 'MainBanner' with (
$banner-height: 357px,
$banner-height: 368.8px,
Copy link
Member

Choose a reason for hiding this comment

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

Giving this a fractional value will cause issues as OSs/browsers do subpixel rendering differently, and it's considered to not be a best practice in CSS. I would recommend leaving it at 357px, and reducing the font size of the text in the message in both banners.

Copy link
Member

Choose a reason for hiding this comment

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

You can actually look at the en banner for this, as that also has a load of text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We had the slider break point increased at 1550 as suggested by the PM.

@Sperling-0 Sperling-0 force-pushed the C24_WMDE_Desktop_DE_22 branch 2 times, most recently from 021e3b1 to 9ccad6b Compare December 20, 2024 11:45
- Var has a different copy
- Adjust the banner height, so that both the banners have same height
- Make slider breakpoint at larger width
- Adjust the tests accordingly
Ticket: https://phabricator.wikimedia.org/T382301
@Sperling-0 Sperling-0 force-pushed the C24_WMDE_Desktop_DE_22 branch from 7828641 to 36f51e3 Compare December 20, 2024 14:53
@moiikana moiikana self-requested a review December 20, 2024 16:31
@moiikana moiikana merged commit a5635a0 into main Dec 20, 2024
1 check passed
@moiikana moiikana deleted the C24_WMDE_Desktop_DE_22 branch December 20, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants