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

Store: Product Trash Pickup #21810

Merged
merged 5 commits into from
Jan 25, 2018
Merged

Store: Product Trash Pickup #21810

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64fefad
Fix #20774: SKU Pencil doesn't activate edit mode.
justinshreve Jan 23, 2018
19942a5
Fix crash that occurs when clicking "add product" from the update scr…
justinshreve Jan 24, 2018
aa2d729
Show a proper error message when a duplicate SKU is found. (16218)
justinshreve Jan 24, 2018
f461134
Fix product name encoding entities (21036)
justinshreve Jan 24, 2018
7189965
Fix tests
justinshreve Jan 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions client/extensions/woocommerce/app/products/product-create.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import { head } from 'lodash';
import { head, isNumber } from 'lodash';
import { localize } from 'i18n-calypso';
import page from 'page';

Expand Down Expand Up @@ -70,12 +70,10 @@ class ProductCreate extends React.Component {
};

componentDidMount() {
const { product, site } = this.props;
const { site } = this.props;

if ( site && site.ID ) {
if ( ! product ) {
this.props.editProduct( site.ID, null, {} );
}
this.props.editProduct( site.ID, null, {} );
this.props.fetchProductCategories( site.ID );
}
}
Expand Down Expand Up @@ -190,6 +188,10 @@ class ProductCreate extends React.Component {
const isBusy = Boolean( actionList ); // If there's an action list present, we're trying to save.
const saveEnabled = isValid && ! isBusy && 0 === this.state.isUploading.length;

if ( ! product || isNumber( product.id ) ) {
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 groking the check for ! product but the second clause has me thinking a bit - so this is to short circuit the create view if a product exists and is not a placeholder/un-saved product?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.

I found in testing, the few actions that clear away edits in product UI state, variations, etc (which also resets currentlyEditingId and this product object) might not have all ran yet before the component mounts. And because of how the CompactTinyMCE component works, (it only an initial value), this is needed to prevent rendering of the form in that brief moment of time. Once the prop updates the form renders.

return null;
}

return (
<Main className={ className } wideLayout>
<ProductHeader
Expand Down
5 changes: 2 additions & 3 deletions client/extensions/woocommerce/app/products/product-form-details-card.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import React, { Component } from 'react';
import config from 'config';
import i18n from 'i18n-calypso';
import PropTypes from 'prop-types';
import { trim, debounce, isNumber } from 'lodash';
import { trim, isNumber } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -45,7 +45,6 @@ export default class ProductFormDetailsCard extends Component {

this.setName = this.setName.bind( this );
this.setDescription = this.setDescription.bind( this );
this.debouncedSetDescription = debounce( this.setDescription, 200 );
Copy link
Contributor

Choose a reason for hiding this comment

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

When trying to track down this bug, this did jump out at me - what was the original reason for this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure now, sorry. It doesn't seem necessary from the testing I did though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was added in here #14944 though i don't see any details around that specific bit. I think this might be some carry-over from patterns in the post editor maybe. But I agree if its not needed, it is great to remove it.

}

// TODO: Consider consolidating the following set functions
Expand Down Expand Up @@ -108,7 +107,7 @@ export default class ProductFormDetailsCard extends Component {
return (
<CompactTinyMCE
initialValue={ product.description || '' }
onContentsChange={ this.debouncedSetDescription }
onContentsChange={ this.setDescription }
/>
);
};
Expand Down
29 changes: 22 additions & 7 deletions client/extensions/woocommerce/components/compact-tinymce/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import React, { Component } from 'react';
import PropTypes from 'prop-types';
import { assign, noop, uniqueId } from 'lodash';
import { assign, noop, uniqueId, forEach } from 'lodash';
import classNames from 'classnames';
import tinymce from 'tinymce/tinymce';
import 'tinymce/themes/modern/theme.js';
Expand Down Expand Up @@ -61,9 +61,12 @@ class CompactTinyMCE extends Component {
}

const { initialValue, onContentsChange } = this.props;

editor.on( 'init', () => {
this.editorContainer = this.editor.getContainer();
this.editor.setContent( wpautop( initialValue ) );
} );

if ( onContentsChange ) {
editor.on( 'change', () => {
onContentsChange( this.editor.getContent() );
Expand Down Expand Up @@ -92,7 +95,7 @@ class CompactTinyMCE extends Component {
browser_spellcheck: true,
fix_list_elements: true,
keep_styles: false,
textarea: this.refs.text,
textarea: this.textarea,
preview_styles: 'font-family font-size font-weight font-style text-decoration text-transform',
end_container_on_empty_block: true,
statusbar: false,
Expand Down Expand Up @@ -121,16 +124,28 @@ class CompactTinyMCE extends Component {

componentWillUnmount() {
this.mounted = false;
if ( this.editor ) {
this.destroyEditor();
}
this.destroyEditor();
}

destroyEditor() {
tinymce.remove( this.editor );
if ( this.editor ) {
forEach(
[ 'change', 'keyup', 'setcontent', 'init' ],
function( eventName ) {
this.editor.off( eventName );
}.bind( this )
);
}

this.editorContainer && tinymce.remove( this.editorContainer );
this.editor = null;
this.editorContainer = null;
}

setTextAreaRef = ref => {
this.textarea = ref;
};

localize() {
const user = userFactory();
const userData = user.get();
Expand All @@ -155,7 +170,7 @@ class CompactTinyMCE extends Component {
const className = classNames( 'compact-tinymce', this.props.className );
return (
<div className={ className }>
<textarea ref="text" className={ tinyMCEClassName } id={ this._id } />
<textarea ref={ this.setTextAreaRef } className={ tinyMCEClassName } id={ this._id } />
</div>
);
}
Expand Down