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

People: First pass at disabling the invite form until invitee added #3172

Merged
merged 2 commits into from
Feb 24, 2016

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Feb 8, 2016

Previously, a user could land on the /people/new/$site route and submit the form right away. This change ensure that there is at least one username or email address before allowing submission.

In the near future, we'll also want to handle these conditions.

  • Are all invitations valid?
  • What happens after a form is submitted? Disable the form until another username or email is added?
  • What if localStorage was cleared and site is not set?

@ebinnion ebinnion self-assigned this Feb 8, 2016
@ebinnion ebinnion added this to the People Management: m7 milestone Feb 8, 2016
@ebinnion ebinnion force-pushed the update/people-invites-disable-button branch from eed4030 to f79d23c Compare February 12, 2016 22:53
@ebinnion
Copy link
Contributor Author

@lezama – I think this is ready to go for now. I'd like to iterate on what happens after submitting the form and handling localStorage being cleared in another PR.

I will create other issues for those.

@ebinnion ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 21, 2016
@ebinnion ebinnion force-pushed the update/people-invites-disable-button branch from 8a40840 to 5417626 Compare February 21, 2016 18:08

// If there are invitees, and there are no errors, let's check
// if there are any pending validations.
return some( usernamesOrEmails, ( value ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would read clearer if we use every instead of some.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We quickly discussed this in Slack, and the condition would be ! every( /* Logic here */ ). I'm not sure that that is more readable. So, we're going to go ahead and ship.

@lezama lezama added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 24, 2016
@lezama
Copy link
Contributor

lezama commented Feb 24, 2016

LGTM if you don't feel like my suggestion is an improvement :)

ebinnion added a commit that referenced this pull request Feb 24, 2016
…e-button

People: First pass at disabling the invite form until invitee added
@ebinnion ebinnion merged commit 81f4653 into master Feb 24, 2016
@ebinnion ebinnion deleted the update/people-invites-disable-button branch February 24, 2016 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants