Skip to content

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Feb 3, 2017

Fixes #6222

Changes proposed in this Pull Request:

  • Connection cards were moved to the bottom of At a Glance
  • General tab is now removed
  • Updated NavigationSettings GUI tests.
  • added GUI tests for DashConnections.

Testing instructions:

  • test in connected and dev mode and make sure the Connections cards work properly.

@eliorivero eliorivero added Admin Page React-powered dashboard under the Jetpack menu [Status] Needs Review This PR is ready for review. labels Feb 3, 2017
@eliorivero eliorivero added this to the Settings UI milestone Feb 3, 2017
@eliorivero eliorivero self-assigned this Feb 3, 2017
@eliorivero eliorivero force-pushed the update/move-connections-to-at-a-glance branch from 836042a to dd1d726 Compare February 3, 2017 21:02
@MichaelArestad
Copy link
Contributor

If you switch them to be DashItem components, you can probably remove a bunch of those positioning styles.

@eliorivero
Copy link
Contributor Author

Updated to use DashItem.

@eliorivero eliorivero force-pushed the update/move-connections-to-at-a-glance branch from 7d7a4d0 to 0cdf2de Compare February 3, 2017 22:14
@beaulebens
Copy link
Member

Looking pretty good, but 2 things;

  1. I got an error in my console (this was on a site in DEV mode), see screenshot below.
  2. Can we use the Mystery Man default for the Gravatar, so that we're not getting some random retro-generated avatar.

screen shot 2017-02-03 at 3 41 10 pm

screen shot 2017-02-03 at 3 42 42 pm

@beaulebens
Copy link
Member

Would look like this with the ?d=mm for the Gravatar. Unless we have/want a better default?

screen shot 2017-02-03 at 3 43 31 pm

@beaulebens
Copy link
Member

For some reason, my head is squashed :)

screen shot 2017-02-03 at 3 47 34 pm

@beaulebens
Copy link
Member

And I don't know if it's related to this specific PR, but when I click the Manage Connection link, and get the dialog, I think there's an unbalanced div or something, because the entire UI (see in the background/shaded part) jumps to the left:

screen shot 2017-02-03 at 3 48 25 pm

@beaulebens
Copy link
Member

Opening that modal also triggers a JS error:

Warning: Modal.getDOMNode(...) is deprecated. Please use ReactDOM.findDOMNode(instance) instead.

@beaulebens
Copy link
Member

beaulebens commented Feb 3, 2017

As a non-Admin (in this case, an Editor) I can't see the connection cards at all. I should probably be able to see my own Account Connection card at least, right? Maybe both (without the Manage Connection link for the site)?

screen shot 2017-02-03 at 3 57 02 pm

@eliorivero
Copy link
Contributor Author

eliorivero commented Feb 6, 2017

Thanks for the review!

Updates:

  • always use mystery man
  • solve JS warning
  • fix user avatar dimensions
  • visible for non-admin users

The issue where the body jumps towards left was fixed and is in latest dops master. You might need to do a yarn build or a npm up (in jetpack directory).

For the warning logged when the modal is launched, I've opened a PR in dops:
Automattic/dops-components#86

@beaulebens
Copy link
Member

Looking/working much better now!

I have 3 more questions, but they might be better handled in different PRs:

  1. Since we're showing the Site Icon for the site there on the site connection side, should we have a link or something to Customizer so that you can change that icon?
  2. Same question for Gravatar?
  3. On the Account connection side, it felt a little bit weird that there's no way to change/manage that connection. Since we're presenting them separately like this, they feel like very separate concepts (like I should be able to connect the site on the one hand, and my user account on the other/separately). Should we have some "Manage Account" option over there? Do we need it? What would that look like/how would it work?

What you've got on this PR looks good to merge for now though, and we can discuss/look at these questions elsewhere.

@MichaelArestad
Copy link
Contributor

Since we're showing the Site Icon for the site there on the site connection side, should we have a link or something to Customizer so that you can change that icon? Same question for Gravatar?

Maybe in another PR, but definitely not needed for this.

On the Account connection side, it felt a little bit weird that there's no way to change/manage that connection. Since we're presenting them separately like this, they feel like very separate concepts (like I should be able to connect the site on the one hand, and my user account on the other/separately). Should we have some "Manage Account" option over there? Do we need it? What would that look like/how would it work?

There is. Just not for site owners.

@MichaelArestad
Copy link
Contributor

@beaulebens tested and confirmed 3

image

image

@MichaelArestad
Copy link
Contributor

@eliorivero Visually, this looks good. Once reviewed by someone else, should be good to go!

@MichaelArestad
Copy link
Contributor

Note: I'll probably be tweaking the styles a little bit next week in another PR.

@eliorivero eliorivero force-pushed the update/move-connections-to-at-a-glance branch from 841220b to b7fe648 Compare February 13, 2017 17:39
@eliorivero eliorivero merged commit 4336cae into feature/settings-overhaul Feb 13, 2017
@eliorivero eliorivero deleted the update/move-connections-to-at-a-glance branch February 13, 2017 18:13
@eliorivero eliorivero removed the [Status] Needs Review This PR is ready for review. label Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants