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(ras-acc): make auth flow UI text filterable #3070

Merged
merged 4 commits into from
Apr 20, 2024

Conversation

chickenn00dle
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1206943664367847/1205669646691019/f

This PR makes the previously hard-coded reader activation auth flow text filterable:

Screenshot 2024-04-17 at 13 01 39

Note: I have not made any of the errors, basic input labels and placeholders, or Google signin related text filterable. Let me know if you think any of these should be added.

How to test the changes in this Pull Request:

  1. Go through the sign in and registration flows and confirm none of the text is broken
  2. Add the following filter and test the sign in and registration flows again. Confirm all of the text has been replaced as expected:
add_filter( 'newspack_reader_activation_auth_labels', function( $labels ) {
	return [
		'invalid_email'           => 'banana',
		'invalid_password'        => 'banana',
		'invalid_display'         => 'banana',
		'blocked_popup'           => 'banana',
		'code_resent'             => 'banana',
		'create_account'          => 'banana',
		'signin'                  => [
			'title'           => 'banana',
			'success_title'   => 'banana',
			'success_message' => 'banana',
			'continue'        => 'banana',
			'resend_code'     => 'banana',
			'otp'             => 'banana',
			'forgot_password' => 'banana',
			'create_account'  => 'banana',
			'register'        => 'banana',
			'go_back'         => 'banana',
			'set_password'    => 'banana',
		],
		'register'                => [
			'title'               => 'banana',
			'success_title'       => 'banana',
			'success_description' => 'banana',
		],
		'verify'                  => 'banana',
		'magic_link'              => 'banana',
		'password_reset_interval' => 'banana',
		'account_link'            => [
			'signedin'  => 'banana',
			'signedout' => 'banana',
		],
		'newsletters'             => 'banana',
	];
} );
  1. Replace the above filter with the following and test the sign in and registration flows one more time. This time confirm all of the default text appears as expected:
add_filter( 'newspack_reader_activation_auth_labels', function( $labels ) {
	return [];
} );

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?

@chickenn00dle chickenn00dle requested a review from a team as a code owner April 17, 2024 17:07
@chickenn00dle chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Apr 17, 2024
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.

Left one suggestion.

}

if ( ! $key ) {
return array_merge( $default_labels, self::$reader_auth_labels );
Copy link
Member

Choose a reason for hiding this comment

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

Kind of a nit: this should be a deep merge, if the filter is to be used as suggested in the PR description. With the following use, the whole signin key is replaced, and sub-keys' values other than title are empty:

		add_filter(
			'newspack_reader_activation_auth_labels',
			function( $labels ) {
				return [
					'signin' => [
						'title' => 'banana',
					],
				];
			}
		);

The proper use of the filter in this case should be:

		add_filter(
			'newspack_reader_activation_auth_labels',
			function( $labels ) {
				$labels['signin']['title'] = 'banana';
				return $labels;
			}
		);

Just pointing this out since there's an array_merge in use, suggesting all defaults will be merged with what's supplied via the filter.

Copy link
Contributor Author

@chickenn00dle chickenn00dle Apr 18, 2024

Choose a reason for hiding this comment

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

Good suggestion!

Rather than perform a deep merge on every call to get_reader_auth_labels though, I decided to adjust the logic for setting the labels cache to account for both usages of the filters above. Let me know what you think:

5b42975

@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 Apr 18, 2024
@dkoo
Copy link
Contributor

dkoo commented Apr 18, 2024

🍌🍌🍌

@chickenn00dle chickenn00dle force-pushed the feat/ras-acc-make-ui-text-filterable branch from c69db08 to 5b42975 Compare April 18, 2024 16:03
@chickenn00dle chickenn00dle merged commit 507fd5a into epic/ras-acc Apr 20, 2024
8 checks passed
@chickenn00dle chickenn00dle deleted the feat/ras-acc-make-ui-text-filterable branch April 20, 2024 16:09
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.6.0-epic-ras-acc.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.9.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.1.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.2.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.4.0-epic-ras-acc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @epic/ras-acc [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.

4 participants