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

Merge company or person and social profiles in onboarding wizard #12563

Conversation

abotteram
Copy link
Contributor

@abotteram abotteram commented Apr 2, 2019

Summary

This PR can be summarized in the following changelog entry:

  • Merged company or person and social profiles in onboarding wizard.

Relevant technical choices:

  • Added react-select for selecting a WordPress user.

Test instructions

Designs followed: https://github.com/Yoast/design/issues/315

This PR can be tested by following these steps:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Requires Yoast/javascript#184
Requires Yoast/javascript#193

Fixes #12516
Fixes #12519

@afercia
Copy link
Contributor

afercia commented Apr 4, 2019

Not sure why the select in the Settings page is using an aria-label instead of a visible <label> element, which is always preferable.

There's already some visible text that can be used as a <label>:

Screenshot 2019-04-04 at 12 11 55

Visible <label> elements benefit all users, for example they're clickable and allow to easily focus the associated form control.

Also, the current aria-label doesn't match the visible text (whether it's a label or not):
aria-label="Organization or person"

Speech input users will try to voice a command based on the visible text e.g. "Click Choose whether the site represents..." and that won't work.

Overall, I'd recommend to just use the existing visible text as a <label> element.

Also, the visible text and the default option text are pretty much the same and a bit repetitive:

  • Choose whether the site represents an organization or a person.
  • Choose whether you're an organization or a person

I'd consider to simplify at least the default option.

Can't test the configuration wizard as there's something wrong in my build and I'm getting JS errors.

@afercia
Copy link
Contributor

afercia commented Apr 4, 2019

Re: react-select: worth noting it's not accessible. See #10555 (comment) and https://github.com/Yoast/my-yoast/issues/1530 where it was first noted, as MyYoast is the place where react-select was used first.

Edit: on MyYoast the last usages of react-select were replaced with native styled <select> in https://github.com/Yoast/my-yoast/pull/2213

js/src/components/WordPressUserSelector.js Outdated Show resolved Hide resolved
js/src/components/WordPressUserSelector.js Outdated Show resolved Hide resolved
js/src/components/WordPressUserSelectorSearchAppearance.js Outdated Show resolved Hide resolved
@@ -52,40 +52,67 @@ exports[`UpsellBox renders the snippet editor inside of it 1`] = `
</a>
test
</p>
<UpsellBox__StyledList
<.UpsellBox__StyledList-gjf66g-0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there a . in front of this component?

Copy link
Contributor Author

@abotteram abotteram Apr 5, 2019

Choose a reason for hiding this comment

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

This is probably a bug in jest-styled-components. We should upgrade to 7.* as soon as it is released (it is currently in beta).

@abotteram
Copy link
Contributor Author

Merging with consent of @moorscode

@abotteram abotteram merged commit d896b05 into feature/person-schema Apr 8, 2019
@abotteram abotteram deleted the 12516-merge-company-or-person-and-social-profiles-in-onboarding-wizard branch April 8, 2019 11:03
@IreneStr IreneStr added this to the 11.0 milestone Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants