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

People: Swap out select elements for radio buttons for Role #31821

Merged
merged 5 commits into from
Apr 3, 2019
Merged

People: Swap out select elements for radio buttons for Role #31821

merged 5 commits into from
Apr 3, 2019

Conversation

sixhours
Copy link
Contributor

@sixhours sixhours commented Mar 27, 2019 •
edited
Loading

Changes proposed in this Pull Request

  • This PR swaps out the Role select element for radio buttons.
  • We'd discussed replacing the select element with the Dropdown component, but using a native form element is better for accessibility.
  • Since there are five (or fewer) roles, the radio buttons present all of the options upfront rather than hiding four of them behind the select menu.

Before

Screen Shot 2019-03-27 at 3 49 10 PM

Screen Shot 2019-03-27 at 5 58 52 PM

After

Screen Shot 2019-03-27 at 3 48 43 PM

Screen Shot 2019-03-27 at 5 58 22 PM

Testing instructions

  • Switch to this PR
  • Go to /people/new/ and ensure you can add a new user to your site with the correct role.
  • Go to /people/edit and edit a user on your site to change their role.

Fixes #31638

@sixhours sixhours self-assigned this Mar 27, 2019
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/role-select-to-radio

@sixhours sixhours changed the title Swap out select component for radio component for role selector. People: Swap out select elements for radio buttons for Role Mar 27, 2019
@sixhours sixhours added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. People Management [Status] Needs e2e Testing and removed [Status] In Progress labels Mar 28, 2019
@sixhours sixhours removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 28, 2019
@sixhours
Copy link
Contributor Author

Looks like we'd need to update e2e tests to push this change through in lib/pages/invite-people-page.js

sixhours added a commit to Automattic/wp-e2e-tests that referenced this pull request Mar 29, 2019
@sixhours
People: Update CSS selectors for Role to match Automattic/wp-calypso#…
5aaded9 
…31821
@sixhours sixhours mentioned this pull request Mar 29, 2019
Open
@sixhours
Copy link
Contributor Author

Updated the test in a PR: Automattic/wp-e2e-tests#1828

@bsessions85
Copy link
Contributor

Hi @sixhours, The e2e tests are now in this same wp-calypso repo. Go ahead and make your e2e test changes in this branch and everything will pass 😄

@sixhours sixhours requested a review from a team as a code owner March 29, 2019 22:48
@sixhours
Copy link
Contributor Author

@bsessions85 😮 That's so awesome. Giving it a try now!

@sixhours sixhours force-pushed the update/role-select-to-radio branch from bbdb9ce to a932a78 Compare April 1, 2019 17:48
@sixhours sixhours added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs e2e Testing labels Apr 2, 2019
@scruffian
Copy link
Member

LGTM 🚢

@scruffian scruffian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 3, 2019
@sixhours sixhours merged commit ad2e4bd into master Apr 3, 2019
@sixhours sixhours deleted the update/role-select-to-radio branch April 3, 2019 14:47
@jancavan jancavan mentioned this pull request May 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scruffian scruffian scruffian approved these changes

Assignees

@sixhours sixhours

Labels
FixTheFlows People Management
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sixhours @matticbot @bsessions85 @scruffian