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: Refresh invite UI with Form components #3058

Merged
merged 7 commits into from
Feb 4, 2016

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Feb 3, 2016

This PR is meant as a possible alternative to #3046.

There are potential issues with moving the form into the section header. And as we explore that in #3046, it makes sense to compare to a form on its own route.

The main benefit here is more space. This space allows us to do things such as provide explanations and labels for the fields. We could also potentially show an inline notice below the token field to provide a bit more explanation as to why a username or email didn't validate.

invite-route

@ebinnion ebinnion added [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR People Management labels Feb 3, 2016
@ebinnion ebinnion self-assigned this Feb 3, 2016
@ebinnion ebinnion added this to the People Management: m7 milestone Feb 3, 2016
@rickybanister
Copy link

This makes me sad because it's not as instant and slick as the 'micro' interface in the SectionHeader, but this likely makes more long-term sense. We can ship this quickly and move on, and think about implementing the design as more of a v2. I'll keep thinking about this. In the meantime there are a few things that would be nice to improve with this PR.

Namely:

  • Can we create a 'light colored' prop for the token field? It would let you just type usernames and emails more like the original design (but with the 'x' next to them to clear)?
  • How can we do nice inline validation in the token field? Can we? I think submitting and getting an error notice for email validation stinks.
  • We need a page title giving a bit more context to this page
  • We'll want to improve some of the label language and things, add some more helper text. I can help with this.
  • A general concern—the role dropdown is a many-to-one relationship here. My early early designs for this page allowed you to invite several people with different roles in one go (but it was overly complicated). Are we ok with letting people add multiple people to a single role?

Nice work on providing a quick alternative @ebinnion

@ebinnion
Copy link
Contributor Author

ebinnion commented Feb 4, 2016

Can we create a 'light colored' prop for the token field?

I think we definitely can. Although, it may be best to break this out in a separate PR.

How can we do nice inline validation in the token field?

We now have a validation endpoint, so there shouldn't be a technical issue here.

I would suggest that we add statuses, similar to how notices work, to the tags. When we combine this with the isBorderless prop as suggested in Slack chat, perhaps the text color is all that changes?

We need a page title giving a bit more context to this page

I'll go ahead and add that.

We'll want to improve some of the label language and things, add some more helper text. I can help with this.

👍

A general concern—the role dropdown is a many-to-one relationship here. My early early designs for this page allowed you to invite several people with different roles in one go (but it was overly complicated). Are we ok with letting people add multiple people to a single role?

I would say that we're OK with it as that is what currently happens in wp-admin. Also, as far as I know, the invitation system currently expects that each batch of invites is for a single role.

@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 4, 2016
@lezama
Copy link
Contributor

lezama commented Feb 4, 2016

Looks good to 🚢 as base for future work 👍

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.

5 participants