Skip to content

Support User: Add user interface components #3032

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

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Conversation

jordwest
Copy link
Contributor

@jordwest jordwest commented Feb 3, 2016

This adds a keyboard shortcut and login box for the support user feature. Props to @mrheston and @jmdodd for the original work on this.

Ref p195om-2GO-p2

cc @dllh

@jordwest jordwest added [Status] In Progress [Feature Group] Support All things related to WordPress.com customer support. labels Feb 3, 2016
@jordwest jordwest self-assigned this Feb 3, 2016
@jordwest jordwest force-pushed the add/support-user/ui branch 2 times, most recently from fed9a51 to 11a2fe0 Compare February 3, 2016 08:47
@jordwest jordwest 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 3, 2016
@@ -119,6 +124,7 @@ Layout = React.createClass( {
<div className={ sectionClass }>
{ config.isEnabled( 'keyboard-shortcuts' ) ? <KeyboardShortcutsMenu /> : null }
{ this.renderMasterbar() }
{ config.isEnabled( 'support-user' ) ? <SupportUser /> : null }
Copy link
Member

Choose a reason for hiding this comment

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

This could be { config.isEnabled( 'support-user' ) && <SupportUser /> }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@dllh
Copy link
Member

dllh commented Feb 3, 2016

I tested this a little, and it works well for me. As noted elsewhere, I do see this js error when I use the feature: "Warning: Failed propType: Invalid prop user of type array supplied to Gravatar, expected object. Check the render method of MeSidebarNavigation."

I think that's not related to this PR and that it's related to the issue reported in #3029.

@jordwest jordwest force-pushed the add/support-user/ui branch 3 times, most recently from 8ae0046 to 3ac1391 Compare February 4, 2016 06:50
@jordwest
Copy link
Contributor Author

jordwest commented Feb 4, 2016

Thanks for the feedback. I've made a few changes in response.
@dllh: I'm now tracking that issue in #3066

@dllh
Copy link
Member

dllh commented Feb 4, 2016

@mtias mind giving this another look to confirm that your suggestions are adequately addressed?

@dllh
Copy link
Member

dllh commented Feb 9, 2016

I took another look at the code, and it looks good to me and seems that the requested changes have been made. I also gave it another test drive, and other than some known issues recorded elsewhere, it works well. Two pieces of feedback that I think we should jot down elsewhere rather than trying to piggyback on this PR:

  • Can we focus the form field on its invocation?
  • Should the masterbar de-highlight be delayed until state is returned to normal?

Let's 🚢.

@dllh dllh 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 Feb 9, 2016
@jordwest jordwest force-pushed the add/support-user/ui branch from 3ac1391 to 315d033 Compare February 9, 2016 11:53
@mtias
Copy link
Member

mtias commented Feb 9, 2016

The new directory placement looks good to me.

@jordwest jordwest force-pushed the add/support-user/ui branch from 315d033 to a3ac6fe Compare February 9, 2016 12:06
@jordwest
Copy link
Contributor Author

jordwest commented Feb 9, 2016

Can we focus the form field on its invocation?

I've opened #3201 for this

Should the masterbar de-highlight be delayed until state is returned to normal?

Currently the highlight changes at the moment the support token injection changes - meaning any API calls from that point will be injected/not injected with the token. I think delaying the change could open us up to instances where the user falsely believes they are acting without/without the support token being injected. What we could do is to place an overlay across the whole page, preventing interaction until all data has been refreshed.

jordwest added a commit that referenced this pull request Feb 9, 2016
Support User: Add user interface components
@jordwest jordwest merged commit dde2cc0 into master Feb 9, 2016
@jordwest jordwest deleted the add/support-user/ui branch February 9, 2016 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Support All things related to WordPress.com customer support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants