Site Picker: Scroll to selected site when opening picker

[nylen]

This PR seeks to accomplish the same goal as #1507: making it easier to select the current site in the site picker.

Here we do that by scrolling to the selected site when the sites picker is loaded or updated:

Sidebar	Masterbar

We intentionally show half of the previous site (if any) to make it visually obvious that there are more sites available if you scroll up.

I've also added a highlight to the selected site in the sites picker that appears when you click the "New Post" icon in the masterbar. I used the same selected colors as the sidebar. Marking as Needs Design Review because of that, and because the hover effects in particular need some love:

Plus a bit of trash pickup along the way:

• Fix the site highlight for sites like nylen.io/blog by matching against site slug correctly. Currently we have both this.props.sites.selected and this.props.selected, we should probably get rid of one of these for clarity. cc @mtias - looks like you're working on changes in this area of the code.
• Correctly clip sites in the masterbar picker to the border-radius of the popover. This is done by setting the scrollable parent to overflow: hidden (reference).

[folletto]

This is looking good! :)

I was thinking to tweak the position we scroll back it to, but I played a bit with different mechanics and in the end I feel tha the one you choose that scrolls up to half of the previous item works well.

Design wise: 👍

[mtias]

I think this was here to make the component self-sufficient in a way. cc @aduth

[aduth]

I think this was here to make the component self-sufficient in a way. cc @aduth

I honestly don't recall, though I'd agree that could be an argument for keeping it the way it was. Was there a reason this change was made?

[nylen]

Yes: <SitesPopover> needs to either use a sites list coming from a parent who is going to observe it and pass changes down to us, or observe it by itself. I don't have a preference on which way that is accomplished, this way just seemed simpler at the time.

[aduth]

Rather than pulling in react-dom to use the findDOMNode method, you could alternatively use a ref on the rendered root div, which would make this method a bit simpler and lose the extra dependency:

scrollIntoView() {
    const parentScrollTop = this.refs.site.offsetTop;

    if ( this.refs.site.previousSibling ) {
        parentScrollTop -= this.refs.site.previousSibling.offsetHeight / 2;
    }

    this.refs.site.parentNode.scrollTop = parentScrollTop;
},

render() {
    return <div ref="site" className={ siteClass } />;
}

[nylen]

I like that better too. Done in caafe71.

[nylen]

@folletto I was hoping a designer could take a look at the hover styles in particular. To clarify, there are a couple of problems:

1. very different hover styles between sidebar and masterbar; the masterbar new post hover looks a bit garish and bright compared to the newer and more subdued sidebar
2. selected takes precedence over hover in sidebar, but hover takes precedence over selected in masterbar new post; these should be consistent

[shaunandrews]

Looking at #1507 it seems like there are a few issues at play here:

1. People want to create a new post (from the Masterbar) for the site they've already selected in My Sites.
2. Once you open the site picker in My Sites, there's no clear way to get back to the sidebar.

Updating the scroll position to bring the selected site into view kind of solves the first issue, but doesn't have much of an affect on the second issue—its still not obvious that clicking the selected site brings you back.

I think the second issue is solved with @nb's suggestion:

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.

Keeping that Switch Site button visible between the sidebar and the picker is super obvious, and gives you a clean "way out."

I do have one issue with the magic scrolling: How it behaves as you move around Calypso. For instance, when you're on Reader, none of this stuff happens in the Masterbar's new post button. I get why, but most people won't understand the distinction.

[nylen]

The main problem with moving sites around in the list is that you break the consistency of the order of the list, which imposes additional cognitive burden on the user. This is even worse for typical users with 2 or 3 sites - keep in mind that a12s with our 50-100+ sites are a bit of an edge case.

From the sidebar picker, there are already two ways to get back to the page you were on:

1. Click anywhere in the overlay that appears over the page content.
2. Scroll to the selected site and click it.

Keeping that Switch Site button visible between the sidebar and the picker is super obvious, and gives you a clean "way out."

I'm not sure I follow. Neither PR makes that change. They both accomplish the same thing - ensuring that the selected site is always visible. This one by scrolling, and #1507 by rearranging the list.

On the Reader or any other "All My Sites" view, there is no currently selected site. We could add some logic to select a default site for the new post button, but that should be a separate change.

[folletto]

very different hover styles between sidebar and masterbar; the masterbar new post hover looks a bit garish and bright compared to the newer and more subdued sidebar

Yeah, I noticed that too. In the sidebar we have two different states for selected and hover, I'd use the same pattern, keeping the blue for hover (since that's a standard for our white controls) but use a subdued look for the pre-selected one. Or, which might be better, at least as the initial step: we can skip pre-highlighting in the dropdown: in the end the scrolling position might be enough (let's just add the class, so styling will be quick and can be prototyped).

selected takes precedence over hover in sidebar, but hover takes precedence over selected in masterbar new post; these should be consistent

That's... true. I've no preference there. But might be another reason to start with no highlight in the masterbar, just scroll, and then we'll see?

[nylen]

Oh, I wasn't aware that it was a standard hover style. Let's leave it alone for now then.

I think the highlight on the selected site is important to keep. In daa4e7d I've made it take precedence over the hover style so we are consistent there.

Rebased after recent masterbar changes (#631) - this is a good improvement that I've wished for on other branches today and I'd like to merge it soon. Anyone want to give a final review?

[folletto]

Reviewing it... I'm not 100% satisfied with the default state because the dropdown has a different scope than the sidebar: the sidebar highlight exists because selecting it is a "null" operation: you are already on it. In the dropdown instead you can actively select that, the selected state is not just an option, but THE option you're likely to take.

Which leads me to say:

1. I think this isn't a blocker, so we can merge it. But: I'd keep the :hover ability to override the selected state, since the intent / UI scope is different.
2. We'll have to iterate on that default highlight.

[nylen]

OK, I've removed that commit so we are back to the previous behavior now. I probably spent too much time on these silly hover styles anyway. Thanks for taking another look.

[enejb]

I just tested this and it works really well! I was going to say that we forgot to document the readme.md with the new props. but it looks like we don't have such documentation at all. 😿 That part could be done in a new PR.