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

Site Picker: Show selected site first #1507

Closed
wants to merge 2 commits into from
Closed

Conversation

nb
Copy link
Member

@nb nb commented Dec 11, 2015

Especially when adding a new post I almost always want to post to the
current site I have worked with – looked at posts, stats, etc.

It doesn’t make much of a difference in the sidebar picker, so I didn’t
exempt it from the change.

Note: in some cases (opening the reader directly) we don’t have the
selected site yet and we can’t show it. For now I decided not to fetch
it if missing.

Testing instructions:

  • Open My Sites
  • Change your site to a different one from your primary
  • Click on Add New post
  • You should see the selected site at the top

Especially when adding a new post I almost always want to post to the
current site I have worked with – looked at posts, stats, etc.

It doesn’t make much of a difference in the sidebar picker, so I didn’t
exempt it from the change.

Note: in some cases (opening the reader directly) we don’t have the
selected site yet and we can’t show it. For now I decided not to fetch
it if missing.
@nb nb added Site Picker [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 11, 2015
@@ -375,7 +375,7 @@ SitesList.prototype.getSite = function( siteID ) {
// clashes between a domain redirect and a Jetpack site, as well as domains
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to move the comment as well to the new function. Since it gives it a bit of a better idea why we are doing the comparison.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you’re totally right, @enejb.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9edad5b.

@enejb
Copy link
Member

enejb commented Dec 11, 2015

One side effect of this is that after selecting a site in the sidebar, next time you open the sidebar the site that you selected is not in the same position but at the very top.

Which I think it is ok but a bit strange, because the sites move around in the sidebar. Could we maybe make this behaviour more dependent on a prop in the site selector?

Since I think it is a really good improvement in the site selector for creating new post or page.

Other then that it works as expected. 👍

@nb nb added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Dec 12, 2015
@nb
Copy link
Member Author

nb commented Dec 12, 2015

One side effect of this is that after selecting a site in the sidebar, next time you open the sidebar the site that you selected is not in the same position but at the very top.

After using this branch for a bit, I found having the selected site on top in the sidebar picker very useful, because there isn’t another easy way to go back after clicking ← Switch Site.

Could we maybe make this behaviour more dependent on a prop in the site selector?

To be honest, I’d rather not add too many options :) Added Needs Design Review label instead.

@folletto
Copy link
Contributor

It's not ideal to shuffle items around because breaks the ability to memorize, and makes any temporary change (i.e. switching once and then going back to the usual pattern) breaking it further.

The correct approach in this case should lean to find a way to solve the issue while preserving the mental model. For example, we cando a light highlight (lol — light gray background for example) and have the selector pre-scrolled to the latest position it was.

This covers properly all the needs:

  • The latest site selected is immediately accessible
  • The order isn't shuffled, so doesn't break memorized patterns

@nylen
Copy link
Contributor

nylen commented Dec 15, 2015

The high-level goal of making it easier to get back to the current site is something I've often wanted to do, but I agree with @folletto that we should do this via scroll position and a highlight, instead of rearranging the sites list. I took this approach in #1641.

I think rearranging sites is even more of a cognitive burden for the typical user who only has 2 or 3 sites. Better to have them stay in the same place with a clear highlight.

From a code debt perspective this is also a cleaner approach. Here are the methods currently in SitesList:

$ perl -ne '/^SitesList.prototype.(.*) =/ and print "$1\n"' client/lib/sites-list/list.js | sort
canManageAnyJetpack
canManageSelectedOrAll
canUpdateFiles
createSiteObject
fetch
fetchAvailableUpdates
get
getJetpack
getLastSelectedSite
getNetworkSites
getPrimary
getPublic
getSelectedOrAll
getSelectedOrAllJetpackCanManage
getSelectedOrAllWithPlugins
getSelectedSite
getSite
getUpgradeable
getVisible
hasSiteWithPlugins
initialize
isConnectedSecondaryNetworkSite
isSelected
markCollisions
onUpdatedPlugin
parse
propagateChange
removeSite
resetSelectedSite
select
selectAll
setSelectedSite
transaction
update
updateSite

We already have lots of methods around selected sites, like getSelectedOrAll, getSelectedOrAllJetpackCanManage,
getSelectedOrAllWithPlugins, etc. And at least three different ways of setting the selected site (select, selectAll, setSelectedSite).

IMO adding miscellaneous methods to SitesList has become a bit of an antipattern for us, and should be avoided if possible.

// clashes between a domain redirect and a Jetpack site, as well as domains
// on subfolders, but we also need to look for the `domain` as a last resort
// to cover mapped domains for regular WP.com sites.
return site.ID === siteID || site.slug === siteID || site.domain === siteID || site.wpcom_url === siteID;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't write this, but... 😢

@enejb
Copy link
Member

enejb commented Jan 8, 2016

I think this PR might not be needed any more since #1641 fixes some of the issues that this PR was solving.

@apeatling
Copy link
Member

@nb Okay to close?

@nb
Copy link
Member Author

nb commented Jan 25, 2016

Yeap, @nylen fixed it all in #1641.

@nb nb closed this Jan 25, 2016
@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 25, 2016
@lancewillett lancewillett deleted the update/selected-site-first branch February 15, 2016 16:43
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants