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

Themes: Leave the thanks modal's Visit Site button disabled until theme #8842

Merged
merged 2 commits into from
Oct 19, 2016

Conversation

kwight
Copy link
Contributor

@kwight kwight commented Oct 18, 2016

switch is complete.

This will also give a few more moments for Headstart processes to complete.

Before:
screen shot 2016-10-18 at 4 24 15 pm

After:
screen shot 2016-10-18 at 4 27 56 pm

Testing

  • Go to /design and activate a new theme.
  • The Visit Site button should be disabled (light blue), and then become active when the throbbing stops and the theme info loads.

@matticbot
Copy link
Contributor

matticbot commented Oct 18, 2016

@kwight kwight added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 18, 2016
@folletto
Copy link
Contributor

Tested on Chrome. Design wise I think it's good 👍

@budzanowski
Copy link
Contributor

👍

@budzanowski budzanowski added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@@ -44,6 +44,10 @@ const ThanksModal = React.createClass( {
page( this.props.site.URL );
},

visitSiteDisabled() {
return this.props.hasActivated ? false : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just

return ! this.props.hasActivated;

(and maybe even inline that expression instead of having a method for it?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just like the fact that method name explains its usage in this place.

@kwight
Copy link
Contributor Author

kwight commented Oct 19, 2016

@ockham Sure, I'll buy that; it's simple enough to grok, and unlikely to ever expand into needing a proper method.

@kwight kwight added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Ready to Merge labels Oct 19, 2016
@ockham ockham added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@ockham
Copy link
Contributor

ockham commented Oct 19, 2016

:shipit:

switch is complete.

This will also give a few more moments for Headstart processes to complete.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants