My Account Sign In. #2590

revanth0212 · 2020-07-30T20:03:25Z

Description

Added singin section to the new My account trigger in the header section.

Related Issue

Closes PWA-625

Verification Stakeholders

@soumya-ashok
@schensley
@jimbo

Verification Steps

Go to the deployed app and click on the Sign In trigger in the header.
Sign in should work as expected.

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2020-07-30T20:18:13Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-625.

Generated by 🚫 dangerJS against 30c8295

devops-pwa-codebuild · 2020-07-30T20:28:24Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2590.pwa-venia.com : LH Performance Expected 0.85 Actual 0.34, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.93, WPT Cache Expected 90 Actual 32
https://pr-2590.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.39, LH Best Practices Expected 1 Actual 0.93
https://pr-2590.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.56, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.93, WPT Cache Expected 65 Actual 44

schensley · 2020-08-05T14:49:18Z

@revanth0212 this looks really good. UX approved.

packages/peregrine/lib/talons/SignIn/useSignIn.js

packages/peregrine/lib/talons/Header/useAccountTrigger.js

packages/peregrine/lib/talons/AuthModal/useAuthModal.js

packages/peregrine/lib/talons/SignIn/useSignIn.js

packages/peregrine/lib/talons/Header/useAccountTrigger.js

sirugh · 2020-08-05T21:21:46Z

packages/peregrine/lib/talons/Header/useAccountTrigger.js

+         * signed in.
+         */
+        if (!isUserSignedIn) {
+            setView(VIEWS.SIGNIN);


What if I'm creating an account, get an error, and then accidentally close the dropdown by clicking outside of it? When I go to re-open the form through the account trigger all of my input values will be gone! I think this needs some re-thinking. Since state is persistent we could keep whatever was previously opened as the rendered child and provide navigation buttons similar to how the left drawer nav works with account chip.

It was a UX requirement to reset the view when the dropdown is closed. The point you raised it reasonable. @schensley any thoughts on this?

packages/peregrine/lib/talons/SignIn/useSignIn.js

packages/venia-ui/lib/components/AccountMenu/accountMenu.js

packages/venia-ui/lib/components/AuthModal/authModal.js

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

packages/venia-ui/lib/components/Header/accountTrigger.js

packages/peregrine/lib/talons/Header/useAccountTrigger.js

revanth0212 · 2020-08-10T19:17:20Z

packages/peregrine/lib/talons/Header/useAccountMenu.js

-            } else {
-                setView('SIGNIN');
-            }
+        if (isUserSignedIn) {


Instead of writing 2 if cases, 1 for accountMenuIsOpen is true and 1 for false, i took the approach of adding accountMenuIsOpen to dependency array even though it is not used in the effect. This works but a new approach we haven't seen before in our code base. Let me know what you guys think.

We do this in the affect immediately above this with location.pathname. And you've already left a comment to that affect above here so I'm fine with it.

I think the technique is fine. Good idea.

Not sure I would have chosen to reset the view to Sign In when the menu opens or closes—I might have preferred to leave it on the current view—but if that's what UX wants I'm not going to push to change it here.

packages/peregrine/lib/talons/Header/useAccountMenu.js

packages/venia-ui/lib/components/SignIn/signIn.js

sirugh · 2020-08-10T19:34:17Z

packages/peregrine/lib/talons/Header/useAccountMenu.js

-            } else {
-                setView('SIGNIN');
-            }
+        if (isUserSignedIn) {


We do this in the affect immediately above this with location.pathname. And you've already left a comment to that affect above here so I'm fine with it.

packages/peregrine/lib/talons/Header/useAccountMenu.js

packages/venia-ui/lib/components/AccountMenu/accountMenuItems.css

packages/peregrine/lib/talons/Header/useAccountTrigger.js

schensley · 2020-08-10T19:43:40Z

@sirugh I take your point generally speaking for clearing a form isn't always ideal (actually it depends on the expectations of the user), in this case account sign-in needs only a few bits of information, and the shopper is not required to do so to use the site. Persisting the error once the module is closed and re-opened seems to give the impression that either the shopper has been locked out or sign-in is broken. I think adding navigation elements to the module would introduce clutter and confusion to an otherwise simple and straightforward interaction.

packages/peregrine/lib/talons/Header/useAccountMenu.js

packages/venia-ui/lib/components/AccountMenu/__tests__/accountMenu.spec.js

sirugh

Approving! There are some spelling corrects to make, but besides that, good job! Thanks for the perseverance here, I know I sent you back and forth with the error handling stuff.

…Menu.spec.js Co-authored-by: Stephen <sirugh@users.noreply.github.com>

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

jimbo

This looks pretty good now. 👍

jimbo · 2020-08-12T17:26:10Z

packages/venia-ui/lib/components/AccountMenu/__tests__/__snapshots__/accountMenu.spec.js.snap

+
+exports[`it renders SignIn component when the view is SIGNIN 1`] = `
+<aside>
+  <Singin Component />


Snapshot probably needs to be updated.

jimbo · 2020-08-12T17:40:41Z

packages/peregrine/lib/talons/Header/useAccountMenu.js

-            } else {
-                setView('SIGNIN');
-            }
+        if (isUserSignedIn) {


I think the technique is fine. Good idea.

Not sure I would have chosen to reset the view to Sign In when the menu opens or closes—I might have preferred to leave it on the current view—but if that's what UX wants I'm not going to push to change it here.

jimbo · 2020-08-12T17:41:44Z

...es/venia-ui/lib/components/AccountMenu/__tests__/__snapshots__/accountMenuItems.spec.js.snap

-  "<Link>Communications</Link>",
-  "<Link>Account Information</Link>",
+<div>
+  &lt;Link&gt;Order History&lt;/Link&gt;


What's going on here? 🤔

dpatil-magento · 2020-08-12T20:44:41Z

@revanth0212 Resolve the conflicts pls.

…_singin_trigger

revanth0212 · 2020-08-12T21:07:50Z

@revanth0212 Resolve the conflicts pls.

Done dude.

revanth0212 added 3 commits July 29, 2020 16:03

Added singin trigger content.

94915e9

Tests update.

d9ace43

Removed forgot password and create account comps.

b53c886

revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jul 30, 2020

m2-community-project bot added this to Ready for Review in Pull Request Progress Jul 30, 2020

Reverting unnecessary changes.

73ad9be

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jul 30, 2020

revanth0212 added 5 commits July 30, 2020 15:35

Converted VIEWS to const export instead of prop.

3c93713

Updated account menu tests.

5c7a95e

Added useAccountTrigger tests.

8220961

Added formErrors support.

d834672

Minor UX changes.

4db9dd6

revanth0212 marked this pull request as ready for review July 31, 2020 21:02

Merge branch 'develop' into revanth/my_account_singin_trigger

10d3a6a

sirugh reviewed Aug 5, 2020

View reviewed changes

packages/peregrine/lib/talons/SignIn/useSignIn.js Outdated Show resolved Hide resolved

m2-community-project bot assigned sirugh Aug 5, 2020

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Aug 5, 2020

m2-community-project bot added the Progress: review label Aug 5, 2020

sirugh reviewed Aug 5, 2020

View reviewed changes

packages/peregrine/lib/talons/SignIn/useSignIn.js Outdated Show resolved Hide resolved

sirugh reviewed Aug 5, 2020

View reviewed changes

packages/peregrine/lib/talons/Header/useAccountTrigger.js Outdated Show resolved Hide resolved

m2-community-project bot assigned revanth0212 Aug 5, 2020

sirugh reviewed Aug 5, 2020

View reviewed changes

packages/peregrine/lib/talons/Header/useAccountTrigger.js Outdated Show resolved Hide resolved

sirugh reviewed Aug 5, 2020

View reviewed changes

Update packages/peregrine/lib/talons/Header/useAccountTrigger.js

5954897

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

revanth0212 force-pushed the revanth/my_account_singin_trigger branch from b471bbc to 5954897 Compare August 5, 2020 22:03

sirugh reviewed Aug 10, 2020

View reviewed changes

packages/venia-ui/lib/components/Header/accountTrigger.js Outdated Show resolved Hide resolved

packages/peregrine/lib/talons/Header/useAccountTrigger.js Outdated Show resolved Hide resolved

Minor.

9c6e0e9

revanth0212 commented Aug 10, 2020

View reviewed changes

sirugh requested changes Aug 10, 2020

View reviewed changes

revanth0212 mentioned this pull request Aug 10, 2020

My Account - Forgot Password #2619

Merged

1 task

revanth0212 and others added 5 commits August 11, 2020 11:01

Updated account menu CSS.

78960e7

Doc updates.

a79f33f

Merge branch 'develop' into revanth/my_account_singin_trigger

0bed2c7

Updated tests.

5dfb835

Updated docs.

8ec88f9

sirugh reviewed Aug 12, 2020

View reviewed changes

packages/peregrine/lib/talons/Header/useAccountMenu.js Outdated Show resolved Hide resolved

sirugh reviewed Aug 12, 2020

View reviewed changes

packages/venia-ui/lib/components/AccountMenu/__tests__/accountMenu.spec.js Outdated Show resolved Hide resolved

sirugh previously approved these changes Aug 12, 2020

View reviewed changes

revanth0212 and others added 2 commits August 12, 2020 11:30

Merge branch 'develop' into revanth/my_account_singin_trigger

da76ae6

Update packages/venia-ui/lib/components/AccountMenu/__tests__/account…

d6f62a0

…Menu.spec.js Co-authored-by: Stephen <sirugh@users.noreply.github.com>

revanth0212 dismissed sirugh’s stale review via d6f62a0 August 12, 2020 17:18

Update packages/peregrine/lib/talons/Header/useAccountMenu.js

64cd9fa

Co-authored-by: Stephen <sirugh@users.noreply.github.com>

sirugh previously approved these changes Aug 12, 2020

View reviewed changes

jimbo reviewed Aug 12, 2020

View reviewed changes

m2-community-project bot assigned jimbo Aug 12, 2020

Merge remote-tracking branch 'origin/develop' into revanth/my_account…

cc00d37

…_singin_trigger

revanth0212 dismissed sirugh’s stale review via cc00d37 August 12, 2020 21:07

Fixed tests.

30c8295

jimbo approved these changes Aug 13, 2020

View reviewed changes

dpatil-magento merged commit 38b6c67 into develop Aug 13, 2020

dpatil-magento deleted the revanth/my_account_singin_trigger branch August 13, 2020 20:50

m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Aug 13, 2020

m2-community-project bot removed the Progress: review label Aug 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

My Account Sign In. #2590

My Account Sign In. #2590

revanth0212 commented Jul 30, 2020 •

edited

PWAStudioBot commented Jul 30, 2020 •

edited

devops-pwa-codebuild commented Jul 30, 2020 •

edited

schensley commented Aug 5, 2020

sirugh Aug 5, 2020

revanth0212 Aug 10, 2020 •

edited

revanth0212 Aug 10, 2020

sirugh Aug 10, 2020

jimbo Aug 12, 2020

sirugh Aug 10, 2020

schensley commented Aug 10, 2020

sirugh left a comment

jimbo left a comment

jimbo Aug 12, 2020

jimbo Aug 12, 2020

jimbo Aug 12, 2020

dpatil-magento commented Aug 12, 2020

revanth0212 commented Aug 12, 2020

My Account Sign In. #2590

My Account Sign In. #2590

Conversation

revanth0212 commented Jul 30, 2020 • edited

Description

Related Issue

Verification Stakeholders

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jul 30, 2020 • edited

devops-pwa-codebuild commented Jul 30, 2020 • edited

schensley commented Aug 5, 2020

Choose a reason for hiding this comment

revanth0212 Aug 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schensley commented Aug 10, 2020

sirugh left a comment

Choose a reason for hiding this comment

jimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Aug 12, 2020

revanth0212 commented Aug 12, 2020

revanth0212 commented Jul 30, 2020 •

edited

PWAStudioBot commented Jul 30, 2020 •

edited

devops-pwa-codebuild commented Jul 30, 2020 •

edited

revanth0212 Aug 10, 2020 •

edited