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

ASUB-8549 custom buttom for facebook and apple - updating user accoun… #2176

Merged

Conversation

LauraPinilla
Copy link
Contributor

@LauraPinilla LauraPinilla commented Jul 2, 2024

…t management

Be sure to run npm test, npm run lint, and write detailed test steps before requesting reviewers

Description

Updates on Social-sign-on & account management block

https://arcpublishing.atlassian.net/browse/ASUB-8521
https://arcpublishing.atlassian.net/browse/ASUB-8549

There are some AppSec Security alerts, All these will be fixed as part of https://arcpublishing.atlassian.net/browse/ASUB-8539

Test Steps

Use the cdn endpoint https://stagingva-staging-sandbox.api.cdn.arcpublishing.com
Since this tenant has all the Third Auth Providers.

2 Blocks has been updated as part of this PR
Create a page with a Social-Sign-on block. Enable/disable the custom field customButtons. It will render the buttons provided by Fb or Google or the custom buttons included as part of this ticket.
Create a page with account-management block. There you should be able to see the identities attached, or you should be able to attach new identities.

  1. Checkout this branch git checkout ASUB-8521-SocialMediaButtons
  2. Run fusion repo with linked blocks npx fusion start -f -l @wpmedia/identity-block
  3. Create a page with a Social-Sign-on block. Enable/disable the custom field customButtons. It will render the buttons provided by Fb or Google or the custom buttons included as part of this ticket.
    4 Create a page with account-management block. There you should be able to see the identities attached, or you should be able to attach/remove identities.

@LauraPinilla LauraPinilla requested a review from a team as a code owner July 2, 2024 02:54
@edwardcho1231
Copy link
Contributor

edwardcho1231 commented Jul 9, 2024

Just curious but what's reasoning behind adding custom button flag on the custom fields instead of just changing the existing button? Thanks.

if (customButtons) {
document.getElementById("custom-google-signin-btn").addEventListener("click", () =>
window.google.accounts.id.prompt((notification) => {
if (notification.isNotDisplayed() || notification.isSkippedMoment()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like google doesn't want you to use isNotDisplayed with FedCM roll out.
https://developers.google.
Screenshot 2024-07-09 at 1 37 49 PM

com/identity/gsi/web/guides/fedcm-migration?s=dc#display_moment

Copy link
Contributor

@accbjt accbjt left a comment

Choose a reason for hiding this comment

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

@LauraPinilla I left some comments and can we go over how we can test this.

/>
</div>
</div>
<SocialSignOnEditableFieldContainer
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here you don't need to use the ternary.

{facebookAppId && (
 <div>Your html here</div>
)}

document.getElementById("custom-Facebook-signin-btn").addEventListener("click", () => {
window.onFacebookSignOn();
});
setTimeout(() => attachEventListener(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for this timeout if it has 0 milliseconds?

@accbjt
Copy link
Contributor

accbjt commented Jul 10, 2024

Just curious but what's reasoning behind adding custom button flag on the custom fields instead of just changing the existing button? Thanks.

@edwardcho1231 I think @LauraPinilla wanted to keep the old version just in case the user wanted the old version. I think we can keep it for now just in case a client wants to use the service providers version of it.

@edwardcho1231
Copy link
Contributor

LGTM

accbjt
accbjt previously approved these changes Jul 12, 2024
@accbjt accbjt added ready for review The PR author has completed the PR template and is ready for a review additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete labels Jul 12, 2024
@LauraPinilla LauraPinilla removed the additional review The PR author has requested multiple reviewers. Do not merge until at least 2 approvals are complete label Jul 16, 2024
blocks/identity-block/intl.json Outdated Show resolved Hide resolved
blocks/identity-block/package.json Show resolved Hide resolved
@malavikakoppula malavikakoppula added needs changes The reviewer has requested changes from the PR author and removed ready for review The PR author has completed the PR template and is ready for a review labels Jul 22, 2024
Copy link
Contributor

@nschubach nschubach left a comment

Choose a reason for hiding this comment

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

useEffect cleanup requests mainly...

@LauraPinilla LauraPinilla added ready for review The PR author has completed the PR template and is ready for a review and removed needs changes The reviewer has requested changes from the PR author labels Jul 26, 2024
@LauraPinilla LauraPinilla requested a review from nschubach July 29, 2024 22:12
Copy link
Contributor

@nschubach nschubach left a comment

Choose a reason for hiding this comment

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

Definitely feel better about the cleanup, thanks Laura!

@LauraPinilla LauraPinilla merged commit 957d9ac into arc-themes-release-version-2.5.0 Jul 31, 2024
8 checks passed
@LauraPinilla LauraPinilla deleted the ASUB-8521-SocialMediaButtons branch July 31, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review The PR author has completed the PR template and is ready for a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants