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

feat: update reader registration block to use newspack-ui #3199

Merged
merged 22 commits into from
Oct 10, 2024

Conversation

thomasguillot
Copy link
Contributor

@thomasguillot thomasguillot commented Jun 25, 2024

All Submissions:

Changes proposed in this Pull Request:

The main goal of this PR is to update the Reader Registration block to use Newspack UI, aligning it with the rest of the RAS-ACC flow.

I've removed the styles (Stacked / Columns) as they're no longer relevant, and simplified the "Sign in" field to just a link/button (removing the need for "Already have an account?"), to further align with RAS-ACC.

On the Newspack UI end, I've made a few changes: I've replaced Inter with a new version and added Italic. I've enqueued the script into the editor to make sure the styles are available there.

image

How to test the changes in this Pull Request:

  1. Add a Reader Registration block to a page.
  2. Switch to this branch and make sure the block still looks fine.
  3. Play with the different settings, etc...

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@thomasguillot thomasguillot added the [Status] Needs Review The issue or pull request needs to be reviewed label Jun 25, 2024
@thomasguillot thomasguillot marked this pull request as ready for review June 25, 2024 16:10
@thomasguillot thomasguillot requested a review from a team as a code owner June 25, 2024 16:10
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

This looks much better! Some issues I've found:

  1. After clicking "Sign Up" or "Sign In with Google", the SSO button (Google login) disappears, which causes a layout shift. On the base branch it's greyed out along with the rest of the UI.
  2. Custom Registration success state is not holding well. This is also the case on the base branch, BTW, but it's less broken on this branch:
Editor epic/ras-acc This PR
image image image

@thomasguillot thomasguillot force-pushed the update/reader-registration-block branch 2 times, most recently from b673ea8 to 7dde411 Compare July 16, 2024 12:59
@thomasguillot thomasguillot force-pushed the update/reader-registration-block branch from 19a7555 to 649d36f Compare July 16, 2024 13:46
@thomasguillot thomasguillot force-pushed the update/reader-registration-block branch from f85de41 to ec367b2 Compare July 16, 2024 16:44
@thomasguillot
Copy link
Contributor Author

@adekbadek both issues should be fixed now.

With the Editor styles, I pushed some minor changes but it looked ok to me

Editor Front-end
editor front-end

@adekbadek adekbadek force-pushed the update/reader-registration-block branch from 4b89c0d to 643c992 Compare August 16, 2024 10:36
@adekbadek adekbadek force-pushed the update/reader-registration-block branch from b2dda6f to 4ed74df Compare August 16, 2024 10:42
@laurelfulford
Copy link
Contributor

@adekbadek Thanks for keeping this one moving! It was on my list to follow up on it but I didn't get to it. Let me know if it'd help at all to have a second set of eyes reviewing since you're also making commits 🙂

@laurelfulford
Copy link
Contributor

Hey @adekbadek! I just wanted to follow up on this one again to see if there's anything still blocking it from a thumbs up? I'm happy to help with merge conflicts/a second set of eyes/making outstanding fixes, whatever! I think it'd be a nice refresh with the rest of the RAS-ACC changes -- thanks! 🙂

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Sep 6, 2024
@adekbadek
Copy link
Member

@laurelfulford – sorry it took so long! 🙈 I've added some tweaks and approved the changes. Since I've contributed code, do you want to give it another review?

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thank you @adekbadek! 🙌

It looks like a weird regression happened with the not-block-specific styles at some point, though I don't get how 😬 I picked through and cross checked against PRs, and I think I found everything that needs to be changed back. Let me know if I can help take care of those!

There was also an issue with reCAPTCHA v2, also probably a merge thing -- I think I found the bit that needs to be changed, but I would definitely trust your eyes more than mine if anything else needs to be moved over.

Otherwise, the block itself looks good! The only visual issue I noticed was a weird side-scroll thing when the block's in a prompt and using reCAPTCHA v2, but that's also happening in the registration modal, and should be fixed with this PR: #3401

CleanShot 2024-09-06 at 12 09 04

Just let me know if you have any questions about my comments, and if I can help with anything (we can always loop in another reviewer! 😄 ) Thanks!

src/newspack-ui/scss/elements/_typography.scss Outdated Show resolved Hide resolved
src/newspack-ui/scss/elements/_tables.scss Outdated Show resolved Hide resolved
src/reader-activation-newsletters/style.scss Outdated Show resolved Hide resolved
src/reader-activation-newsletters/style.scss Outdated Show resolved Hide resolved
src/newspack-ui/scss/elements/misc/_reader-auth.scss Outdated Show resolved Hide resolved
src/newspack-ui/scss/elements/forms/_checkbox-radio.scss Outdated Show resolved Hide resolved
src/newspack-ui/scss/elements/forms/_checkbox-radio.scss Outdated Show resolved Hide resolved
includes/templates/reader-activation/login-form.php Outdated Show resolved Hide resolved
src/blocks/reader-registration/index.php Outdated Show resolved Hide resolved
@laurelfulford
Copy link
Contributor

Hey @adekbadek! Sorry for the late follow up, this completely fell off my radar. Is there anything I can do to help with the changes requested for this one? Happy to help make changes, or answer questions -- just let me know, thanks!

@adekbadek
Copy link
Member

Sorry for my late follow up here. I've applied your suggestions – thank you!

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

Thanks @adekbadek! 🙌 I checked the fixes, and you took care of everything I spotted!

I noticed one more small visual thing but it's minor. Once that's taken care of this should be good to go!

h2,
h3 {
h2:not([class*="font-size"]),
h3:not([class*="font-size"]) {
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 like these styles are affecting header Registration Block:

CleanShot 2024-10-07 at 11 53 01

Compared to the mockup:

CleanShot 2024-10-07 at 11 50 08

It looks like the title is set to the large font size here so I think it's just a specificity thing.

The theme uses classes with font-size, but WordPress adds !important to those as part of Gutenberg. Newspack UI just uses font -- I think it may be safe to remove the :not unless this is meant to protect against something else?

Copy link
Member

Choose a reason for hiding this comment

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

These styles were added because the less-specific selectors were overriding font sizes set in the editor:

Editor epic/ras-acc this branch
image image image

I've added !importants in 96afca6, would that be ok?

Copy link
Contributor

@laurelfulford laurelfulford left a comment

Choose a reason for hiding this comment

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

These styles were added because the less-specific selectors were overriding font sizes set in the editor

Ah! Thanks @adekbadek -- I totally missed that 🤦‍♀️

I think !important makes sense here if we want to control the font size that closely -- I'm still not sure if we need the :not() selectors but they're not causing other issues; if they do in the future we can deal with them then 🙂

Thanks for wrangling this one to the finish! It looks good to me!

@adekbadek adekbadek merged commit bfab5b7 into epic/ras-acc Oct 10, 2024
9 checks passed
@adekbadek adekbadek deleted the update/reader-registration-block branch October 10, 2024 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ras-acc testing [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants