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

Site Settings: Hide the delete site notice if the site is not loaded yet #2983

Merged
merged 1 commit into from
Feb 3, 2016

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Feb 2, 2016

This fixes an error reported by @drewblaisdell in D979:

If local storage is empty, before sites-list is loaded, we display a warning without a domain.

image

Testing instructions:

  • Go to http://calypso.localhost:3000/settings/delete-site/:site
  • Clean your localStorage with localStorage.clear();
  • Reload the page
  • Assert that the notice is not displayed until the sites are loaded

Review:

  • Code Review
  • Product Review

@Tug Tug added [Feature] Site Settings All other general site settings. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 2, 2016
@Tug Tug self-assigned this Feb 2, 2016
@Tug Tug added this to the Amber: Backlog milestone Feb 2, 2016
@@ -32,6 +32,10 @@
&.is-info {
background: $blue-wordpress;
}

&.hidden {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem a great way to handle these — we should just not render the notice.

@mtias mtias added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 2, 2016
@@ -113,7 +114,7 @@ module.exports = React.createClass( {
<ActionPanel>
<ActionPanelTitle>{ strings.deleteSite }</ActionPanelTitle>
<ActionPanelBody>
<Notice status="is-warning" showDismiss={ false }>
<Notice status="is-warning" className={ classNames( { hidden: ! site } ) } showDismiss={ false }>
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Matias. We could do something like the following:

{ site &&
  <Notice status="is-warning" showDismiss={ false }>
    { this.translate( '{{strong}}%(domain)s{{/strong}} will be unavailable in the future.', {
      components: {
        strong: <strong />
      },
      args: {
        domain: site.domain
      }
    } ) }
  </Notice>
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that seems better.

@Tug Tug force-pushed the fix/delete-site-notice-display branch from 982081f to ddfa596 Compare February 2, 2016 14:19
@Tug Tug force-pushed the fix/delete-site-notice-display branch from ddfa596 to 01fef7c Compare February 2, 2016 14:20
@Tug
Copy link
Contributor Author

Tug commented Feb 2, 2016

I changed the way the notice is rendered following @mtias advice, PR is ready to be reviewed again!

@Tug Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 2, 2016
@drewblaisdell
Copy link
Contributor

👍

}
} ) }
</Notice>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could reduce the length of this function by doing the opposite - which I find more logic by the way:

renderNotice: function() {
  var site = this.state.site;

  if ( site ) {
    return (
      <Notice status="is-warning" showDismiss={ false }>
        { this.translate( '{{strong}}%(domain)s{{/strong}} will be unavailable in the future.', {
          components: {
            strong: <strong />
          },
          args: {
            domain: site.domain
          }
        } ) }
      </Notice>
    );
  }
},

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if there is a benefit either way, but if it is arbitrary, my preference is for the way @Tug implemented it, because:

  • It works with multiple conditions that bail out early (example from ManagePurchase):
if ( ! isRenewable( purchase ) && ! isRedeemable( purchase ) ) {
    return null;
}

if ( ! isExpired( purchase ) ) {
    return null;
}
  • It's easier (for me) to read JSX that isn't wrapped in an extra block/blocks.

@scruffian
Copy link
Member

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants