Convert Sharing to use es6 #11724
Convert Sharing to use es6 #11724
Conversation
obenland
added
[Feature] Sharing
matticbot
added
the
[Size] XL
|
@aduth Can the |
Yeah, would be a good idea. |
client/my-sites/sharing/main.jsx
Outdated
| var site = sites.getSelectedSite(), | ||
| pathSuffix = sites.selected ? '/' + sites.selected : '', | ||
| filters = []; | ||
| class Sharing extends Component { |
There was a problem hiding this comment.
Given it has no state, would probably be fairly easy to express this as a function component if we're comfortable collapsing getFilters into the render logic.
client/my-sites/sharing/main.jsx
Outdated
| }; | ||
|
|
||
| export default connect( | ||
| ( state, { site } ) => ( { |
There was a problem hiding this comment.
Maybe a little much to refactor in the context of this pull request, but it would be nice to drop the full site object in favor of the more specific selectors: getSiteSlug, isJetpackSite, isJetpackModuleActive, isJetpackMinimumVersion.
| next(); | ||
| }; | ||
|
|
||
| export default { |
There was a problem hiding this comment.
Do we need the default export if we're already exporting the individual functions (appears sharing/index.js should work without the default export).
There was a problem hiding this comment.
Good 👀. I would probably do a usage search to see if we have any usages like require( './controller' ).buttons. This should be safe to remove if we update any requires/imports to import { button } from './controller'; or import * as controller from './controller'
client/my-sites/sharing/main.jsx
Outdated
| return ( | ||
| <Main className="sharing"> | ||
| <DocumentHead title={ this.props.translate( 'Sharing', { textOnly: true } ) } /> |
There was a problem hiding this comment.
I know I've seen this elsewhere, but where is textOnly documented or referenced in code? It's not clear when it should be used (I seem to recall something about being auto-wrapped with span but not sure if that's still the case).
There was a problem hiding this comment.
Thanks for asking that! I wondered myself but not enough to actually research it.
Before React 0.14 when ReactElements became plain objects, the option was needed to override ReactElement.prototype.toString to have control over how the object is converted to a string. The option is unused since #2455, so I'll remove it
I could not be more comfortable with that. See #11724 (comment) 0
matticbot
added
the
[Size] XL
|
I'm unable to load the Sharing Buttons screen from an empty cache, but this is also the case in production, so unlikely to be related to the changes here specifically. |
| import { renderWithReduxStore } from 'lib/react-helpers'; | ||
| import { sectionify } from 'lib/route'; | ||
| import Sharing from 'my-sites/sharing/main'; |
There was a problem hiding this comment.
Minor: Seems like in this and following imports it would be reasonable to import relative:
import Sharing from './main';
client/my-sites/sharing/main.jsx
Outdated
|
|
||
| return filters; | ||
| return { | ||
| showButtons: ( siteId && canCurrentUser( state, siteId, 'manage_options' ) && ( ! isJetpack || ( |
There was a problem hiding this comment.
This is quite the condition :D Not sure how else we might simplify it aside from maybe splitting off a few variables: canManageOptions ? supportsSharingButtons ?
Not sure why it used absolute paths before. See #11724 (comment) 0
Pulls out some of the function calls to use more concise variables in the condition See #11724 (comment) 5
matticbot
added
the
[Size] XL
matticbot
removed
the
[Status] Needs Review
None of the props are actually required, and all queries can happen one we do have a site ID. See #11724 (comment) 38
None of the props are actually required, and all queries can happen one we do have a site ID. See #11724 (comment) 38
This PR modernizes Sharing's top-level components and removes the feature flag.
After #8991 these are the remaining pieces that can be cleaned up.
To test:
Verify that My Sites > Sharing is still accessible, both the Connections and the Sharing Buttons tab.