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.

Sign up for GitHub

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

Jump to bottom

833 refactor the create account form #835

Merged
merged 14 commits into from
May 10, 2019
Merged

833 refactor the create account form #835

merged 14 commits into from
May 10, 2019

Conversation

VitaliyBoyko
Copy link

@VitaliyBoyko VitaliyBoyko commented Feb 6, 2019 •
edited by sirugh
Loading

Description

PR contains refactoring of createAccount component

Related Issue

Closes #833 [REFACTORING] Refactor the createAccount form

Motivation and Context

In order to follow the principle of least astonishment, I've provided refactoring of createAccount form component, removed hardcoded validation, updated tests.

Screenshots (if appropriate):

image

Proposed Labels for Change Type/Package

REFACTORING
venia-concept

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

@vercel vercel bot temporarily deployed to staging February 6, 2019 13:29 Inactive
@vercel
Copy link

vercel bot commented Feb 6, 2019 •
edited
Loading

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://venia-git-fork-vitaliyboyko-pwa-studio-833-refa-305973.magento-research1.now.sh

@vercel vercel bot temporarily deployed to staging February 6, 2019 13:44 Inactive
@vercel vercel bot temporarily deployed to staging February 6, 2019 13:45 Inactive
@vercel vercel bot temporarily deployed to staging February 7, 2019 07:23 Inactive
@supernova-at supernova-at changed the base branch from release/2.0 to develop March 6, 2019 16:59
@coveralls
Copy link

coveralls commented Apr 25, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 77.987% when pulling 645d29a on VitaliyBoyko:pwa-studio-833-refactor-the-create-account-form into 3cc4deb on magento-research:develop.

@tjwiebell
Copy link
Contributor

@supernova-at - This appears to overlap enough with #913 that one of us would have a terrible conflict resolution ahead of us. Going to mark as hold, and link to #913.

@tjwiebell tjwiebell added the hold On hold until another condition is fulfilled. label Apr 25, 2019
@supernova-at supernova-at mentioned this pull request Apr 25, 2019
Merged
5 tasks
@supernova-at supernova-at self-assigned this Apr 26, 2019
@supernova-at
Copy link
Contributor

Good call 👍.

I tried to hold off changing much about this page in #913 knowing that this PR was coming.

I'll take ownership of it since I was just working in this code.

@awilcoxa awilcoxa removed the hold On hold until another condition is fulfilled. label May 2, 2019
supernova-at
supernova-at reviewed May 3, 2019
View reviewed changes
packages/venia-concept/src/components/CreateAccount/asyncValidators.js
} catch (error) {
throw 'An error occurred while looking up this email address.';
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

The next version of informed that we're moving to in #1078 does not support asyncValidators. We have decided to keep this code around in the hopes that support for it gets added back. Then we can just hook up this code to the UI.

@supernova-at supernova-at removed their assignment May 3, 2019
@supernova-at
Copy link
Contributor

@VitaliyBoyko thanks so much for this! Sorry for the delay; since it has been so long since we looked at this I went ahead and pushed a couple minor commits instead of asking you to change anything.

We'll get this reviewed and merged ASAP 😃

@tjwiebell
Copy link
Contributor

@supernova-at - Please resolve conflicts with latest mainline and look into the unit test failure reported by Danger.

@supernova-at supernova-at added hold On hold until another condition is fulfilled. in progress and removed in progress hold On hold until another condition is fulfilled. labels May 8, 2019
@tjwiebell tjwiebell self-assigned this May 9, 2019
tjwiebell
tjwiebell approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Had some naming comments, but looks like most of this was pulled from existing files, so no big deal. LGTM.

@tjwiebell tjwiebell removed their assignment May 9, 2019
@dpatil-magento dpatil-magento merged commit 0ed9379 into magento:develop May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@supernova-at supernova-at supernova-at left review comments

@tjwiebell tjwiebell tjwiebell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[REFACTORING] Refactor the createAccount form
6 participants
@VitaliyBoyko @coveralls @tjwiebell @supernova-at @dpatil-magento @awilcoxa