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

Me: Use SitesDropdown instead of SelectSite in /me/account #1837

Merged
merged 6 commits into from
Jan 15, 2016

Conversation

ebinnion
Copy link
Contributor

Fixes #451

Now that we have a SitesDropdown, we can retire our use of SelectSite. This component swaps out the former for the latter on the /me/account route.

Note: There is still some jankiness where switching a site and then refreshing the page may not show the proper primary site. This is because the sites list data is stale after we make the change. So, we'll need to find some way to update the primary site in the sites list.

To test:

  • Checkout fix/me-account-site-dropdown
  • Go to /me/account
  • Change primary site
  • Go to wordpress.com/me/account
  • Does primary site reflect the change that you just made?

screen shot 2

@ebinnion ebinnion added [Status] In Progress [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. labels Dec 18, 2015
@ebinnion ebinnion self-assigned this Dec 18, 2015
@ockham
Copy link
Contributor

ockham commented Dec 21, 2015

Now that #1886 has been merged, this is the last occurrence of me/select-site.jsx, so maybe you could also remove that file as part of this PR?

@mtias
Copy link
Member

mtias commented Jan 14, 2016

Nice.

This is because the sites list data is stale after we make the change. So, we'll need to find some way to update the primary site in the sites list.

This is something we'll properly fix once we migrate site-settings to redux and centralize the data. In the meantime you could just set the primary attribute manually, or change the order of the array. This is where we set the attribute.

cc @rralian to keep in mind for settings.

@ebinnion
Copy link
Contributor Author

@mtias - I updated the PR to manually set the primary site. After I did that, I noticed that the old primary site still showed on refresh, which I tracked down to SitesDropdown falling back to the primary site if a site is not specified. So, the primary site was being shown until the /me/settings API call came back.

I addressed by adding an isPlaceholder prop to SitesDropdown which, when true, will display a Site placeholder.

After that, I noticed that the site and chevron could overlap each other. So, I adjusted the styling of the component to use flexbox.

Overlapping:
screen shot 5

Overlapping fixed:

Chrome:
screen shot 8

screen shot 6

IE11:

screen shot 9

screen shot 7

iPhone4

screen shot 4
Safari:

@ockham - I went ahead and removed the select-site component in this PR as well. 👍

@ebinnion ebinnion added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Jan 14, 2016
@rickybanister
Copy link

Looks good to me.

@@ -210,6 +210,13 @@ module.exports = React.createClass( {
} );
},

onSiteSelect( siteSlug ) {
let selectedSite = sites.getSite( siteSlug );
if ( sites.getSite( siteSlug ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

is this a typo? should it be:

if ( selectedSite ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh... you're right 😄 I created the variable so I didn't have to get the site twice, and then I did it anyways 🐼

@dmsnell
Copy link
Member

dmsnell commented Jan 15, 2016

@ebinnion looks good. My only real question was about a possible typo and the data path for updating the primary site.

ebinnion added a commit that referenced this pull request Jan 15, 2016
Me: Use SitesDropdown instead of SelectSite in /me/account
@ebinnion ebinnion merged commit 0016b6e into master Jan 15, 2016
@ebinnion ebinnion deleted the fix/me-account-site-dropdown branch January 15, 2016 22:48
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 15, 2016
@lancewillett lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Me / Site Picker: suggestion: use blog picker styles for primary blog dropdown
7 participants