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.

Sign up for GitHub

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

Jump to bottom

ThemesSiteSelectorModal: Fix siteSlug value of null, take two #9368

Closed
wants to merge 3 commits into from
Closed

ThemesSiteSelectorModal: Fix siteSlug value of null, take two #9368

wants to merge 3 commits into from

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Nov 14, 2016 •
edited
Loading

Have <ThemesSiteSelectorModal /> store only the current option's name (instead of the entire option object) in component state. This way, when Redux state updates, the component's options prop will update and cause a re-render with updated getUrl and action fields.

To test:

Reverts #9367

@ockham ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Nov 14, 2016
@ockham ockham added this to the Themes: JetPress Rally milestone Nov 14, 2016
@ockham ockham self-assigned this Nov 14, 2016
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=revert-9367-revert-9341-fix/themes-site-selector-modal-null-site

@matticbot matticbot added the [Size] S Small sized issue label Mar 30, 2017
@dmsnell
Copy link
Member

dmsnell commented May 6, 2017

@ockham is there a status update on this PR?

@ockham
Copy link
Contributor Author

ockham commented May 8, 2017

@dmsnell I think I can rebase and merge this after #13094 is in...

@dmsnell
Copy link
Member

dmsnell commented Jun 21, 2017

@ockham #13094 is in, want to rebase and merge?

@ockham
Copy link
Contributor Author

ockham commented Jun 21, 2017

I'll try to get to it this week.

@ockham ockham force-pushed the revert-9367-revert-9341-fix/themes-site-selector-modal-null-site branch from 6d9c9b6 to 4987cee Compare June 26, 2017 13:28
@ockham ockham mentioned this pull request Jun 26, 2017
Closed
@ockham
Copy link
Contributor Author

ockham commented Jun 27, 2017

Seems we don't need this to fix #9298 after all now, see #9298 (comment). There's a chance we still need it for #14896, but let's re-evaluate once we get to that.

@ockham ockham closed this Jun 27, 2017
@ockham
Copy link
Contributor Author

ockham commented Jun 27, 2017

Spoke too soon. We need these changes even for a simple UI fix, since selectedOption.hideForTheme is otherwise bound too state while all required information isn't yet available (and doesn't update later, due to the way it's used in wrapOption).

@ockham ockham reopened this Jun 27, 2017
@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Jun 27, 2017
@ockham ockham mentioned this pull request Jun 27, 2017
Closed
@ockham ockham added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 27, 2017
@seear
Copy link
Contributor

seear commented Jun 27, 2017

Testing Notes:

master
The behaviour I see doesn't quite seem to match #9298, but it is certainly broken in that I see no sites or only my primary site in the dropdown.

This branch
The sites load with a proper placeholder and then any of my sites are available for selection from the dropdown. Customizer loads for both .com and jetpack site.

mcsf
mcsf reviewed Jun 27, 2017
View reviewed changes
Copy link
Member

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Raised a couple of questions, but this seems sensible. :)

client/my-sites/themes/themes-site-selector-modal.jsx
@@ -83,12 +83,12 @@ const ThemesSiteSelectorModal = React.createClass( {
* but only if it also has a header, because the latter indicates it really needs
* a site to be selected and doesn't work otherwise.
*/
wrapOption( option ) {
wrapOption( option, name ) {
Copy link
Member

Choose a reason for hiding this comment

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

There are two occurrences in this component of wrapOption being called with one argument. Should we fix them to provide a name? Or have the method default to some value for undefined name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh, good spot. What we need is the corresponding object key. I'll fix that.

client/my-sites/themes/themes-site-selector-modal.jsx
return Object.assign(
{},
option,
option.header
? { action: themeId => this.showSiteSelectorModal( option, themeId ) }
? { action: themeId => this.showSiteSelectorModal( name, themeId ) }
Copy link
Member

Choose a reason for hiding this comment

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

Arguably something to tackle separately, but, since wrapOption is called within the render stack, it would be cool to avoid instantiating functions here — preferably only passing data or, at the very least, preëxisting* bound methods.

*: can't resist them diæreses…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, agree. In a way, that's what I'm fixing with this PR here. If there's an easy solution you can think of off the top of your head, I'd be curious to hear 😄

hoverduck
hoverduck approved these changes Jun 27, 2017
View reviewed changes
Copy link
Contributor

@hoverduck hoverduck left a comment

Choose a reason for hiding this comment

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

Tested, looks good to me

seear
seear approved these changes Jun 27, 2017
View reviewed changes
Copy link
Contributor

@seear seear left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

client/components/site-selector-modal/index.jsx
<Button
primary
key="mainAction"
disabled={ ! siteId }
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, if ! siteId, then url is falsy, so we don't reach this code? On the other hand it is nice the way you use the same ! siteId logic in both branches, so I guess this is fine.

budzanowski
budzanowski approved these changes Jun 27, 2017
View reviewed changes
Copy link
Contributor

@budzanowski budzanowski left a comment

Choose a reason for hiding this comment

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

👍

@ockham
Copy link
Contributor Author

ockham commented Jun 29, 2017

Thanks for testing/reviewing, folks! Unfortunately, the glitch @mcsf discovered isn't quite as trivial to fix as I'd hoped, which is why I haven't merged this PR yet. I'll try to get to it soon.

@gwwar
Copy link
Contributor

gwwar commented Aug 30, 2017

Still need this one @ockham ?

@alisterscott
Copy link
Contributor

We we still planning to merge this one? Thanks

@ockham
Copy link
Contributor Author

ockham commented Oct 31, 2017

I'll try and look into it on Thu!

@apeatling
Copy link
Member

@ockham Any more movement here? This is the oldest open PR in the whole project, we should make a decision to either move on it, or move on.

@ockham
Copy link
Contributor Author

ockham commented Nov 9, 2017

Looks like I can't find the time to move on it. Closing it.

@ockham ockham closed this Nov 9, 2017
@ockham ockham deleted the revert-9367-revert-9341-fix/themes-site-selector-modal-null-site branch November 9, 2017 13:50
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcsf mcsf mcsf left review comments

@hoverduck hoverduck hoverduck approved these changes

@seear seear seear approved these changes

@budzanowski budzanowski budzanowski approved these changes

@mendezcode mendezcode Awaiting requested review from mendezcode

Assignees

@ockham ockham

Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites. [Size] M Medium sized issue [Status] Needs Rebase
Projects
None yet
Milestone
Themes: JetPress Rally
Development

Successfully merging this pull request may close these issues.
10 participants
@ockham @matticbot @dmsnell @seear @gwwar @alisterscott @apeatling @mcsf @hoverduck @budzanowski