Clean/site list/sites dropdown #9624

brunoscopelliti · 2016-11-24T07:28:15Z

Hi everybody,

@gwwar , as you suggested I am opening a pull request to track our work on this request; this is not ready to be merged now.

I've also a couple of questions for you;
I understand that you might be working on completely different things right now, so let me give you a little more context before asking the questions (sorry for the long text).

I am working on the sites-dropdown component.
It permits the user to select what site should be considered as primary. It uses site-selector component for the selection logic, and considers as initially selected the site that is currently the user's primary site (indepentently from what is the currently selected site in the redux store).

Since the component receives the currently primary site as selected prop, we can't use getSelectedSite from 'state/ui/selectors' to get the site that should be initially selected.

Currently I am trying using getSite from 'state/sites/selectors'.
Its signature is ( state, siteId ) => Site. So it works on the basis of the siteId.

const mapStateToProps = ( state, ownProps ) => ( {
	primarySite: getSite( state, ownProps.selected ),
} );

export default connect( mapStateToProps )( SitesDropdown );

Here, the problem is that sometimes the sites-dropdown component is used, specifing as selected prop the siteSlug, instead of that the siteId.
In these cases this PR, at the current state, introduces a bug.

From a quick search in the codebase I found that sites-dropdown is used only in the following components/pages.

# providing selected prop as slug
components/site-selector-modal
me/help/help-contact-form

# providing selected prop as siteId
me/account/main.jsx

My first feeling was that we should uniform, and probably use always the siteId (since selectors work exclusively with it).
I've started doing this but I felt that I was just increasing confusion around siteId, and slug; I've now reverted those changes.
For example site-selector when an option is selected, executes onSiteSelect passing as argument the slug (not the siteId), and this depend more deeper by the site component.

const SiteSelector = React.createClass( {
    ...
    onSiteSelect( event, siteSlug ) {
        const handledByHost = this.props.onSiteSelect( siteSlug );
        ...

At this regard my first question is: what do you consider the best way to identify a site (siteId or slug)?
The codebase is not completely uniform here; for example site-selector works mostly with the slug name. I guess this is due to the fact that selectors are a relatively new introduction, and you want to have uniformity one day.
Eventually I would be happy to help.

A completely different approach would be to make getRawSite work both with siteId, both with slug transparently.
That's surely less work, but I am not sure it is the right thing to do. What do you think?

export const getRawSite = ( state, siteId ) => {
	return state.sites.items[ siteId ] || null;
};

Can you think to other alternatives which haven't come to my mind?

gwwar · 2016-11-29T00:43:32Z

Hi @brunoscopelliti ! It's fine to leave in the sites.getPrimary() call until we have a good way of fetching the primary site in redux. We made one attempt here in #9115 but we probably also need to figure out a way to avoid duplication, probably by rigging up SitesList to treat the redux tree as the source of truth.

At this regard my first question is: what do you consider the best way to identify a site (siteId or slug)?

They're interchangeable with our routes, but I would say that siteId is more reliable programmatically, while a slug is much more human readable.

For example the following will render stats for the same site:

https://wordpress.com/stats/day/gwwartest.wordpress.com
https://wordpress.com/stats/day/102465845

I'm not too familiar with the history of this component. @mtias did you have any extra background for why selected can be siteId or siteSlug, besides ease of use?

For example site-selector when an option is selected, executes onSiteSelect passing as argument the slug (not the siteId), and this depend more deeper by the site component.

In that case I believe many of the callbacks either redirect to a url, or generate links, of which a site slug is much prettier than using the site id.

At this regard my first question is: what do you consider the best way to identify a site (siteId or slug)?

I think any human readable links should see a slug, while siteId is more dependable in internal code. Note that we need to be careful when handling slugs for Jetpack connected sites (#9207) That being said I think it's fair to point out that we'll want to have a selector that returns a site given a slug, especially when we begin to store more than just the selected site in the redux tree.

gwwar · 2016-11-29T00:49:04Z

Also, for context here's the line where we populate selected site in the redux tree. This occurs on any browser refresh, or when you select a site with the picker. https://github.com/Automattic/wp-calypso/blob/master/client/my-sites/controller.js#L172

I'd need to dig a bit in the component, but it might impact this selection loop. If you run into trouble, let us know and I can find a more straightforward component to port.

brunoscopelliti · 2016-11-29T07:42:22Z

Hi @gwwar,

If you are not in hurry to have the sites-selector component migrated away from getSelectedSite, I am happy to continue to work on this.

Thanks to your answers, I think to know how to proceed now... but I've finished my time for today.
Tomorrow I'll write down what I think is the best way to proceed.

brunoscopelliti · 2016-11-30T07:00:32Z

did you have any extra background for why selected can be siteId or siteSlug, besides ease of use?

I scraped the codebase; the only case in which we pass as selected prop the siteId is in the account section, https://github.com/Automattic/wp-calypso/blob/master/client/me/account/main.jsx#L388

There're a couple of other components, which use sites-dropdown.

components/site-selector-modal
me/help/help-contact-form

while siteId is more dependable in internal code.

Agree!
For this reason I'd like to:

Rename sites-dropdown's selected property to selectedSiteId (React.PropTypes.number).
Update components/site-selector-modal, and me/help/help-contact-form, so that pass the siteId to sites-dropdown.

I've already this working on my fork, but I've not pushed yet cause I'm not sure is what we want.

I think that ideally also site-selector (and blocks/site at a deeper level) should use siteId. This will extend greatly the scope of this PR; eventually we could do this in a second step.

I think any human readable links should see a slug,

Agree, again :)
Once our components work consistently with the siteId, I guess it shouldn't be too hard to get the siteSlug from the siteId.
An example, openEditorWithSite will eventually receive the siteId, then using getSite selector could retrieve the site, hence the siteSlug.

That being said I think it's fair to point out that we'll want to have a selector that returns a site given a slug,

This won't be immediately necessary if we decide to use siteId.
It becomes necessary if we decide that sites-dropdown component should continue to work with siteSlug.

@gwwar What do you think?

brunoscopelliti · 2016-11-30T07:04:03Z

I've also a meta question :)

I would like to maintain the branch in sync with master.
Currently I am doing the following, and I would like to know if it is the right flow.

# on master
git pull Automattic master
git push

# on this PR branch
git rebase master
git push -f

gwwar · 2016-11-30T23:42:34Z

Agree!
For this reason I'd like to:
Rename sites-dropdown's selected property to selectedSiteId (React.PropTypes.number).
Update components/site-selector-modal, and me/help/help-contact-form, so that pass the siteId to sites-dropdown.

Sounds reasonable to me though, let's hear from some other folks, maybe @mtias @ockham @aduth @ebinnion . If we do decide to update the contract I would recommend creating new targeted PR(s) to do so.

I've also a meta question :)

The workflow looks good to me. The push force flag is fine to use when you're working alone on a branch. You can also set up the remotes inversely:

# do once:
git clone git@github.com:Automattic/wp-calypso.git
git remote add mine git@github.com:<your user name>/wp-calypso.git
# create a branch
git checkout -b my_branch
git push mine my_branch
# updating a local branch
git fetch origin
git checkout my_branch
git rebase -i origin/master
git push -f mine my_branch

brunoscopelliti · 2016-12-01T06:14:44Z

If we do decide to update the contract I would recommend creating new targeted PR(s) to do so.

It makes sense :)
Since I've already these changes pending on my fork, I'm going to open a new pull request... If @mtias @ockham @aduth @ebinnion think this is not the correct path to follow, then we could simply discard it.

ps. Thanks for the workflow advices!

brunoscopelliti · 2016-12-07T04:53:48Z

@gwwar,

The work on the PR #9754 is completed, and waits a review.
This is how I expect to proceed now... let me know if it sounds good, or have any other advices.

I'll wait that Reinforce use of siteId in sites-dropdown+ancestors components #9754 is merged on master.
Rebase this PR, and complete the work here in order to remove getSelectedSiteId.
In another PR, work on site-selector to replace the internal usages of siteSlug, with siteId.

brunoscopelliti · 2016-12-14T06:58:12Z

I've put down a couple of notes... any other feedback is welcome.

Anyway this is not yet ready to be merged, cause I noticed a strange behaviour.
When I visit directly http://calypso.localhost:3000/me/account, and then I open the dropdown, it renders also the currently selected site, as option.

I've not yet understood the reason for this behaviour, but I'll keep you informed eventually.

Update.

Well, I've reduced further the circumstances in which this behaviour appears.
In order to reproduce the issue you need to be just logged in, and then visit /me/account as first page.

The cause is that in this case the selected prop, that we pass to site-selector, is null.
In this case I think that updating the state on the componentWillReceiveProps hook should do the trick.

I will update the PR as soon as possible.

brunoscopelliti · 2016-12-16T20:33:03Z

I think this PR is now ready to be reviewed; I've left a few comments/questions too.

@gwwar I've read in the other thread that you're going to take a vacation... enjoy it! I'll appreciate your feedback on this, when you will be back.

gwwar · 2017-01-04T20:01:09Z

Hi, @brunoscopelliti do you mind rebasing this branch with master?

gwwar · 2017-01-04T20:11:43Z

client/components/sites-dropdown/test/index.js

-			expect( sitesDropdown.hasClass( 'sites-dropdown' ) ).to.be.true;
-			expect( sitesDropdown.hasClass( 'is-open' ) ).to.be.false;
+		before( function() {
+			ConnectedSitesDropdown = require( '../index.jsx' ).default;


Interesting case here, another option would be to import the unconnected class, and pass through the sites and selected siteId data. @gziolo do you have any opinions here?

We have never tested Component wrapped with connect so far. I'm not opposed to this approach. From what I see in this example you get additional verification for your connect method, which can be useful.

We could also avoid using redux-mock-store by testing a plain component with all props injected when necessary. At the same time we could also export mapStateToProps and test it independently.

My personal preference is the latter option, because it makes testing setup much easier.

gwwar · 2017-01-04T20:15:06Z

package.json

@@ -177,6 +177,7 @@
    "react-addons-test-utils": "15.4.0",
    "react-hot-loader": "1.3.0",
    "react-test-env": "0.2.0",
+    "redux-mock-store": "1.2.1",


If we plan on adding a new dev dependency, don't forget to update the shrinkwrap file. You can do this by navigating to your local checkout and typing make shrinkwrap which handles the process for you. To see what the script does see this doc

brunoscopelliti · 2017-01-05T06:42:26Z

Hi @gwwar,
welcome back :)

I've rebased, and updated npm-shrinkwrap.json.

I guess you should be pretty busy at your return at work; I will appreciate your feedback on the questions still opened. Then we could decide how to proceed.

Refactoring to ES6 class, extending PureComponent Using getSite selector in place of sites-list's getSite Replacing sites.getSite with component prop Make sure that 'sites' is populated in Redux store Restore unit tests Handle component loaded before 'me/sites' request Update components gallery example urlToSlug is a better way to get the site slug Added redux-mock-store to test the connected component Update shrinkwrap Test mapStateToProps instead of the connected component

gwwar · 2017-01-06T22:33:31Z

Yup, I'm still catching up on pings @brunoscopelliti 😄 I'll try to get a proper look at this on Monday.

gwwar · 2017-01-09T21:31:19Z

client/components/sites-dropdown/docs/example.jsx

+
+const getAllSites = ( state ) => state.sites.items;
+
+const getFirstSiteId = ( state ) => Object.keys( state.sites.items )[ 0 ];


We can add these as selectors in state/sites/selectors if we think they'd be useful in an implementation.

gwwar · 2017-01-09T21:37:08Z

client/components/sites-dropdown/docs/example.jsx


-	displayName: 'SitesDropdown',
+		this.state = {
+			selectedSiteId: this.props.selectedSiteId


Why do we need this component state?

gwwar · 2017-01-09T21:40:46Z

client/components/sites-dropdown/docs/example.jsx

+	render(){
+		return (
+			<SitesDropdown
+				selectedSiteId={ this.state.selectedSiteId }


It's probably better to defer to default handling in SitesDropdown than add logic in the example.

gwwar · 2017-01-09T21:43:33Z

client/components/sites-dropdown/index.jsx

@@ -74,14 +79,15 @@ export default class SitesDropdown extends PureComponent {
 	render() {
 		return (
 			<div className={ classNames( 'sites-dropdown', { 'is-open': this.state.open } ) }>
+				<QuerySites allSites={ true } />


This will duplicate sites data in localstorage (SitesLIst) and indexedDB. Let's avoid this for now until we remove more usages of SitesList

gwwar · 2017-01-09T21:44:30Z

client/components/sites-dropdown/index.jsx

 		this.onClose = this.onClose.bind( this );

-		const selectedSite = props.selectedSiteId


We still need to depend on SitesList for primary site until we're a bit further along in the project.

gwwar · 2017-01-09T21:48:37Z

client/components/sites-dropdown/index.jsx

+		if ( primarySite && primarySite.slug !== this.state.selectedSiteSlug ) {
+			this.setState( {
+				selectedSiteSlug: primarySite.slug
+			} );


When we're ready, I would instead move this logic to getSelectedSiteSlug or getSelectedSiteId in state/sites/selectors. Unless there's a special need, this can also be passed through as a connected prop vs setting this as component state.

gwwar · 2017-01-09T22:10:52Z

Thanks @brunoscopelliti, I really appreciate the PR but we're not quite ready for this change-set yet. We want to avoid duplicating sites data in multiple stores, until we're confidant that we can really get rid of SitesList quickly.

One good next step would be to create a PR that adds a memoized selector for getPrimarySite in state/sites/selectors. When we actually have all sites in redux, we can do a quick swap of sites.getPrimary() call with this new selector.

Right now this is handled by SitesList that filters for a primary property and falls back to the first item, if nothing is marked: https://github.com/Automattic/wp-calypso/blob/master/client/lib/sites-list/list.js#L179

matticbot · 2017-02-23T23:52:26Z

@brunoscopelliti This PR needs a rebase

gwwar · 2017-06-07T19:03:13Z

Thanks again @brunoscopelliti for taking a shot at this. With #13094 and #14026 landed, I think we can close out this PR

matticbot added the OSS Citizen label Nov 24, 2016

hoverduck added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 24, 2016

brunoscopelliti mentioned this pull request Dec 1, 2016

Reinforce use of siteId in sites-dropdown+ancestors components #9754

Merged

brunoscopelliti mentioned this pull request Dec 13, 2016

Fix state property name #10000

Merged

gwwar self-requested a review January 4, 2017 19:58

gwwar reviewed Jan 4, 2017

View reviewed changes

gwwar reviewed Jan 9, 2017

View reviewed changes

gwwar mentioned this pull request Jan 9, 2017

Framework: remove sitesList.getSelectedSite from my-sites/current-sites #10481

Merged

matticbot added the [Status] Needs Rebase label Feb 23, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 30, 2017

mcsf mentioned this pull request Apr 20, 2017

SitesDropdown: Partial dumping of SitesList #13278

Merged

ockham mentioned this pull request Apr 20, 2017

SitesDropdown: Use Site IDs internally, drop SitesList from consumers #13293

Merged

gwwar closed this Jun 7, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean/site list/sites dropdown #9624

Clean/site list/sites dropdown #9624

brunoscopelliti commented Nov 24, 2016

gwwar commented Nov 29, 2016

gwwar commented Nov 29, 2016 •

edited

Loading

brunoscopelliti commented Nov 29, 2016

brunoscopelliti commented Nov 30, 2016

brunoscopelliti commented Nov 30, 2016

gwwar commented Nov 30, 2016

brunoscopelliti commented Dec 1, 2016

brunoscopelliti commented Dec 7, 2016

brunoscopelliti commented Dec 14, 2016 •

edited

Loading

brunoscopelliti commented Dec 16, 2016

gwwar commented Jan 4, 2017

gwwar Jan 4, 2017

gziolo Jan 5, 2017

gwwar Jan 4, 2017

brunoscopelliti commented Jan 5, 2017

gwwar commented Jan 6, 2017

gwwar Jan 9, 2017

gwwar Jan 9, 2017

gwwar Jan 9, 2017

gwwar Jan 9, 2017

gwwar Jan 9, 2017

gwwar Jan 9, 2017

gwwar commented Jan 9, 2017 •

edited

Loading

matticbot commented Feb 23, 2017

gwwar commented Jun 7, 2017


		const getAllSites = ( state ) => state.sites.items;

		const getFirstSiteId = ( state ) => Object.keys( state.sites.items )[ 0 ];

		this.onClose = this.onClose.bind( this );

		const selectedSite = props.selectedSiteId

Clean/site list/sites dropdown #9624

Clean/site list/sites dropdown #9624

Conversation

brunoscopelliti commented Nov 24, 2016

gwwar commented Nov 29, 2016

gwwar commented Nov 29, 2016 • edited Loading

brunoscopelliti commented Nov 29, 2016

brunoscopelliti commented Nov 30, 2016

brunoscopelliti commented Nov 30, 2016

gwwar commented Nov 30, 2016

brunoscopelliti commented Dec 1, 2016

brunoscopelliti commented Dec 7, 2016

brunoscopelliti commented Dec 14, 2016 • edited Loading

brunoscopelliti commented Dec 16, 2016

gwwar commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoscopelliti commented Jan 5, 2017

gwwar commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar commented Jan 9, 2017 • edited Loading

matticbot commented Feb 23, 2017

gwwar commented Jun 7, 2017

gwwar commented Nov 29, 2016 •

edited

Loading

brunoscopelliti commented Dec 14, 2016 •

edited

Loading

gwwar commented Jan 9, 2017 •

edited

Loading