Skip to content

Refactor country group switcher to CSS in JS #6645

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

Merged
merged 5 commits into from
Jan 6, 2025

Conversation

andrewHEguardian
Copy link
Contributor

What are you doing in this PR?

Refactor the country group switcher and its subcomponents from scss to emotion.

Trello Card

Why are you doing this?

We no longer use sass in this project.

How to test

Go to https://support.thegulocal.com/uk/contribute and click on the country drop down. Also https://support.thegulocal.com/uk/subscribe/weekly. Test the hover states, and closing and opening the dialog.

Screenshots

Before (3 tier landing)
image

After
image

Before (subscription landing)
image

After
image

.component-country-group-switcher {
display: inline-flex;

.component-select-input__select {
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 could not find this used anywhere, so I did not recreate in emotion

color: inherit;
cursor: pointer;
color: ${palette.neutral[100]};
${textSans17};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous version was size 16, this doesn't exist in source so I went with 17px

Copy link
Contributor

github-actions bot commented Dec 19, 2024

Size Change: +4.72 kB (+0.21%)

Total Size: 2.24 MB

Filename Size Change
./public/compiled-assets/javascripts/[countryGroupId]/router.js 265 kB +591 B (+0.22%)
./public/compiled-assets/javascripts/digitalSubscriptionLandingPage.js 236 kB +579 B (+0.25%)
./public/compiled-assets/javascripts/supporterPlusLandingPage.js 307 kB +551 B (+0.18%)
./public/compiled-assets/javascripts/weeklySubscriptionLandingPage.js 87.7 kB +563 B (+0.65%)
ℹ️ View Unchanged
Filename Size Change
./public/compiled-assets/javascripts/[countryGroupId]/events/router.js 90.1 kB +2 B (0%)
./public/compiled-assets/javascripts/ausMomentMap.js 108 kB +1 B (0%)
./public/compiled-assets/javascripts/contributionsRedirectStyles.js 20 B 0 B
./public/compiled-assets/javascripts/downForMaintenancePage.js 70.6 kB +214 B (+0.3%)
./public/compiled-assets/javascripts/error404Page.js 70.6 kB +215 B (+0.31%)
./public/compiled-assets/javascripts/error500Page.js 70.5 kB +215 B (+0.31%)
./public/compiled-assets/javascripts/favicons.js 617 B 0 B
./public/compiled-assets/javascripts/paperSubscriptionCheckoutPage.js 175 kB +235 B (+0.13%)
./public/compiled-assets/javascripts/paperSubscriptionLandingPage.js 87.4 kB +228 B (+0.26%)
./public/compiled-assets/javascripts/payPalErrorPage.js 68.9 kB +270 B (+0.39%)
./public/compiled-assets/javascripts/payPalErrorPageStyles.js 20 B 0 B
./public/compiled-assets/javascripts/promotionTerms.js 73.5 kB +215 B (+0.29%)
./public/compiled-assets/javascripts/subscriptionsLandingPage.js 72.8 kB +197 B (+0.27%)
./public/compiled-assets/javascripts/subscriptionsRedemptionPage.js 127 kB +201 B (+0.16%)
./public/compiled-assets/javascripts/unsupportedBrowserStyles.js 20 B 0 B
./public/compiled-assets/javascripts/weeklySubscriptionCheckoutPage.js 172 kB +218 B (+0.13%)
./public/compiled-assets/webpack/136.js 2.17 kB 0 B
./public/compiled-assets/webpack/187.js 21.6 kB +227 B (+1.06%)
./public/compiled-assets/webpack/344.js 2.01 kB 0 B
./public/compiled-assets/webpack/643.js 22.4 kB 0 B
./public/compiled-assets/webpack/706.js 107 kB 0 B

compressed-size-action

Copy link
Member

@rupertbates rupertbates left a comment

Choose a reason for hiding this comment

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

LGTM. Great to get rid of this 👏

open ? 'open' : null,
styled ? 'styled' : null,
])}
css={[dialog, open && openCss, styled && styledCss]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Havent used that method before, saves passing the parameter in side the css, great

Copy link
Contributor

@paul-daniel-dempsey paul-daniel-dempsey left a comment

Choose a reason for hiding this comment

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

Looks Good

@andrewHEguardian andrewHEguardian merged commit 8e899fb into main Jan 6, 2025
18 checks passed
@andrewHEguardian andrewHEguardian deleted the ahe/country-group-switcher-css branch January 6, 2025 11:32
@prout-bot
Copy link

Seen on PROD (merged by @andrewHEguardian 10 minutes and 7 seconds ago)

Sentry Release: support-client-side, support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants