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

Auto-save text written in DismissibleTextForm #6884

Merged
merged 5 commits into from
Nov 12, 2018
Merged

Auto-save text written in DismissibleTextForm #6884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9565b44
Auto-save text written in DismissibleTextForm
kumar303 Nov 9, 2018
c604aa7
Clean up tests, bump bundlesize
kumar303 Nov 9, 2018
8e3de9d
Clear the form when dismissing
kumar303 Nov 12, 2018
cec3498
Switch dependency injection to underscore prefix style
kumar303 Nov 12, 2018
fa04ee6
Switch log level to debug
kumar303 Nov 12, 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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@
"bundlesize": [
{
"path": "./dist/@(amo|disco)-!(i18n-)*.js",
"maxSize": "420 kB"
"maxSize": "430 kB"
kumar303 marked this conversation as resolved.
Show resolved Hide resolved
},
{
"path": "./dist/@(amo|disco)-i18n-*.js",
Expand Down
10 changes: 10 additions & 0 deletions src/amo/components/AddonReviewCard/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { ADDONS_EDIT } from 'core/constants';
import { withErrorHandler } from 'core/errorHandler';
import translate from 'core/i18n/translate';
import log from 'core/logger';
import { normalizeFileNameId } from 'core/utils';
import { getCurrentUser, hasPermission } from 'amo/reducers/users';
import {
beginDeleteAddonReview,
Expand Down Expand Up @@ -275,11 +276,20 @@ export class AddonReviewCardBase extends React.Component<InternalProps> {
return null;
}

const formId = [
normalizeFileNameId(__filename),
'addon',
addon ? addon.id.toString() : 'no-addon',
'review',
review ? review.id.toString() : 'unsaved-review',
].join('-');
kumar303 marked this conversation as resolved.
Show resolved Hide resolved

return (
<div className="AddonReviewCard-reply">
{replyingToReview ? (
<DismissibleTextForm
className="AddonReviewCard-reply-form"
id={formId}
isSubmitting={submittingReply && !errorHandler.hasError()}
onDismiss={this.onDismissReviewReply}
onSubmit={this.onSubmitReviewReply}
Expand Down
11 changes: 6 additions & 5 deletions src/amo/components/AddonReviewManager/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
import AddonReviewManagerRating from 'amo/components/AddonReviewManagerRating';
import { withFixedErrorHandler } from 'core/errorHandler';
import translate from 'core/i18n/translate';
import { sanitizeHTML } from 'core/utils';
import { normalizeFileNameId, sanitizeHTML } from 'core/utils';
import DismissibleTextForm from 'ui/components/DismissibleTextForm';
import type { AppState } from 'amo/store';
import type { DispatchFunc } from 'core/types/redux';
Expand All @@ -38,6 +38,10 @@ type InternalProps = {|
flashMessage?: FlashMessageType | void,
|};

export const extractId = (props: Props | InternalProps): string => {
return props.review.id.toString();
};

export class AddonReviewManagerBase extends React.Component<InternalProps> {
static defaultProps = {
puffyButtons: false,
Expand Down Expand Up @@ -137,6 +141,7 @@ export class AddonReviewManagerBase extends React.Component<InternalProps> {
<DismissibleTextForm
dismissButtonText={i18n.gettext('Cancel')}
formFooter={formFooter}
id={`${normalizeFileNameId(__filename)}-${extractId(this.props)}`}
isSubmitting={flashMessage === STARTED_SAVE_REVIEW}
onDismiss={onCancel}
onSubmit={this.onSubmitReview}
Expand All @@ -158,10 +163,6 @@ const mapStateToProps = (state: AppState) => {
};
};

export const extractId = (props: Props): string => {
return props.review.id.toString();
};

const AddonReviewManager: React.ComponentType<Props> = compose(
connect(mapStateToProps),
withFixedErrorHandler({ fileName: __filename, extractId }),
Expand Down
17 changes: 10 additions & 7 deletions src/amo/components/EditableCollectionAddon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { withFixedErrorHandler } from 'core/errorHandler';
import { getAddonIconUrl } from 'core/imageUtils';
import translate from 'core/i18n/translate';
import withUIState from 'core/withUIState';
import { nl2br, sanitizeHTML } from 'core/utils';
import { nl2br, normalizeFileNameId, sanitizeHTML } from 'core/utils';
import Button from 'ui/components/Button';
import DismissibleTextForm from 'ui/components/DismissibleTextForm';
import Icon from 'ui/components/Icon';
Expand Down Expand Up @@ -46,6 +46,11 @@ type InternalProps = {|
uiState: UIStateType,
|};

export const extractId = (ownProps: Props | InternalProps) => {
const { addon } = ownProps;
return `editable-collection-addon-${addon.id}`;
};

export class EditableCollectionAddonBase extends React.Component<InternalProps> {
onEditNote = (event: SyntheticEvent<HTMLElement>) => {
event.preventDefault();
Expand Down Expand Up @@ -95,8 +100,8 @@ export class EditableCollectionAddonBase extends React.Component<InternalProps>
render() {
const { addon, className, errorHandler, i18n } = this.props;
const showNotes = addon.notes || this.props.uiState.editingNote;

const iconURL = getAddonIconUrl(addon);

return (
<li
className={makeClassName(
Expand Down Expand Up @@ -150,6 +155,9 @@ export class EditableCollectionAddonBase extends React.Component<InternalProps>
{errorHandler.renderErrorIfPresent()}
<DismissibleTextForm
className="EditableCollectionAddon-notes-form"
id={`${normalizeFileNameId(__filename)}-${extractId(
this.props,
)}`}
microButtons
onDelete={addon.notes ? this.onDeleteNote : null}
onDismiss={this.onDismissNoteForm}
Expand Down Expand Up @@ -188,11 +196,6 @@ export class EditableCollectionAddonBase extends React.Component<InternalProps>
}
}

export const extractId = (ownProps: Props) => {
const { addon } = ownProps;
return `editable-collection-addon-${addon.id}`;
};

const EditableCollectionAddon: React.ComponentType<Props> = compose(
translate(),
withFixedErrorHandler({ fileName: __filename, extractId }),
Expand Down
3 changes: 2 additions & 1 deletion src/amo/components/ReportUserAbuse/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from 'amo/reducers/userAbuseReports';
import { withErrorHandler } from 'core/errorHandler';
import translate from 'core/i18n/translate';
import { sanitizeHTML } from 'core/utils';
import { normalizeFileNameId, sanitizeHTML } from 'core/utils';
import Button from 'ui/components/Button';
import DismissibleTextForm from 'ui/components/DismissibleTextForm';
import type { AppState } from 'amo/store';
Expand Down Expand Up @@ -134,6 +134,7 @@ export class ReportUserAbuseBase extends React.Component<InternalProps> {
</p>

<DismissibleTextForm
id={normalizeFileNameId(__filename)}
isSubmitting={isSubmitting}
onDismiss={this.hideReportUI}
onSubmit={this.sendReport}
Expand Down
9 changes: 5 additions & 4 deletions src/core/localState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import defaultLocalForage from 'localforage';

import log from 'core/logger';
import { normalizeFileNameId } from 'core/utils';

export function configureLocalForage({
localForage = defaultLocalForage,
Expand All @@ -11,7 +12,7 @@ export function configureLocalForage({
localForage.config({
name: 'addons-frontend',
version: '1.0',
storeName: 'core.LocalState',
storeName: normalizeFileNameId(__filename),
});
}

Expand Down Expand Up @@ -43,14 +44,14 @@ export class LocalState {
return data;
})
.catch((error) => {
log.error(`Error with localForage.getItem("${this.id}"): ${error}`);
log.debug(`Error with localForage.getItem("${this.id}"): ${error}`);
throw error;
});
}

clear(): Promise<void> {
return this.localForage.removeItem(this.id).catch((error) => {
log.error(`Error with localForage.removeItem("${this.id}"): ${error}`);
log.debug(`Error with localForage.removeItem("${this.id}"): ${error}`);
throw error;
});
}
Expand All @@ -62,7 +63,7 @@ export class LocalState {
);
}
return this.localForage.setItem(this.id, data).catch((error) => {
log.error(`Error with localForage.setItem("${this.id}"): ${error}`);
log.debug(`Error with localForage.setItem("${this.id}"): ${error}`);
throw error;
});
}
Expand Down
58 changes: 56 additions & 2 deletions src/ui/components/DismissibleTextForm/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
/* @flow */
import makeClassName from 'classnames';
import { oneLine } from 'common-tags';
import debounce from 'lodash.debounce';
import invariant from 'invariant';
import * as React from 'react';
import { compose } from 'redux';
import Textarea from 'react-textarea-autosize';

import translate from 'core/i18n/translate';
import log from 'core/logger';
import createLocalState, { LocalState } from 'core/localState';
import Button from 'ui/components/Button';
import type { ElementEvent } from 'core/types/dom';
import type { I18nType } from 'core/types/i18n';
Expand All @@ -26,6 +30,7 @@ type Props = {|
className?: string,
dismissButtonText?: string,
formFooter?: React.Element<any>,
id: string,
onDelete?: null | (() => void),
onDismiss?: () => void,
onSubmit: (params: OnSubmitParams) => void,
Expand All @@ -42,6 +47,8 @@ type Props = {|

type InternalProps = {|
...Props,
_createLocalState: typeof createLocalState,
_debounce: typeof debounce,
i18n: I18nType,
|};

Expand All @@ -59,9 +66,13 @@ export class DismissibleTextFormBase extends React.Component<
InternalProps,
State,
> {
localState: LocalState;

textarea: React.ElementRef<typeof Textarea>;

static defaultProps = {
_createLocalState: createLocalState,
_debounce: debounce,
isSubmitting: false,
microButtons: false,
puffyButtons: false,
Expand All @@ -72,19 +83,46 @@ export class DismissibleTextFormBase extends React.Component<
super(props);
const initialText = props.text || '';
this.state = { initialText, text: initialText };
this.localState = this.createLocalState();
}

componentDidMount() {
if (this.textarea) {
this.textarea.focus();
}
this.checkForStoredState();
}

componentDidUpdate(prevProps: InternalProps) {
if (this.props.id !== prevProps.id) {
this.localState = this.createLocalState();
this.checkForStoredState();
}
}

createLocalState() {
const { _createLocalState, id } = this.props;
return _createLocalState(id);
}

async checkForStoredState() {
const storedState: State | null = await this.localState.load();
if (storedState) {
log.debug(
oneLine`Initializing DismissibleTextForm state from LocalState
${this.localState.id}`,
storedState,
);
this.setState(storedState);
kumar303 marked this conversation as resolved.
Show resolved Hide resolved
}
}

onDelete = (event: SyntheticEvent<any>) => {
event.preventDefault();

invariant(this.props.onDelete, 'onDelete() is not defined');
this.props.onDelete();
this.localState.clear();
};

onDismiss = (event: SyntheticEvent<any>) => {
Expand All @@ -93,16 +131,32 @@ export class DismissibleTextFormBase extends React.Component<
invariant(onDismiss, 'onDismiss() is required');

onDismiss();
this.setState({ text: '' });
this.localState.clear();
kumar303 marked this conversation as resolved.
Show resolved Hide resolved
};

onSubmit = (event: SyntheticEvent<any>) => {
event.preventDefault();
this.props.onSubmit({ event, text: this.state.text });
this.localState.clear();
};

persistState = this.props._debounce(
(state) => {
// After a few keystrokes, save the text to a local store
// so we can recover from crashes.
this.localState.save(state);
},
800,
{ trailing: true },
);

onTextChange = (event: ElementEvent<HTMLInputElement>) => {
event.preventDefault();
this.setState({ text: event.target.value });

const newState = { text: event.target.value };
this.setState(newState);
this.persistState(newState);
};

render() {
Expand Down Expand Up @@ -211,14 +265,14 @@ export class DismissibleTextFormBase extends React.Component<
return (
<form className={makeClassName('DismissibleTextForm-form', className)}>
<Textarea
defaultValue={this.state.text}
Copy link
Contributor Author

@kumar303 kumar303 Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I honestly don't know how this worked before but it did. I read the react-textarea-autosize source twice. I have no idea! It no longer works with my new call to setState({ text: ... }) but I don't know what's different. In any case, it should have always been setting the value prop, not defaultValue.

Copy link
Member

Choose a reason for hiding this comment

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

That's an uncontrolled component, no? https://reactjs.org/docs/uncontrolled-components.html#default-values A weird one though, like a semi-controlled/semi-uncontrolled component 😄

Since you can supply any props to Textarea and therefore to the underlying textarea, that defaultValue prop is passed to the textarea and the textarea becomes an uncontrolled component. In theory, one would need to use a ref to read the textarea content but... the Textarea component implements an onChange handler on the underlying textarea.

According to the code, onChange is always called in Textarea: https://github.com/andreypopp/react-textarea-autosize/blob/f4f49e84c4beccf29188c72c2a3b21899bc0e85a/src/index.js#L113, so the onTextChange callback is also always called, and when it updates the state, the defaultValue gets updated too (because of the re-render).

The implementation of onChange in Textarea is not correct IMO, it should early return in if (!this._controlled) { ... }, so that developers would know that defaultValue and onChange don't play nice together.

disabled={isSubmitting}
className="DismissibleTextForm-textarea"
inputRef={(ref) => {
this.textarea = ref;
}}
onChange={this.onTextChange}
placeholder={text.placeholder}
value={this.state.text}
Copy link
Member

Choose a reason for hiding this comment

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

👍

/>
{formFooter && (
<div className="DismissibleTextForm-formFooter">{formFooter}</div>
Expand Down
21 changes: 19 additions & 2 deletions tests/unit/amo/components/TestAddonReviewCard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
fakeReview,
shallowUntilTarget,
} from 'tests/unit/helpers';
import DismissibleTextForm from 'ui/components/DismissibleTextForm';
import ErrorList from 'ui/components/ErrorList';
import LoadingText from 'ui/components/LoadingText';
import UserReview from 'ui/components/UserReview';
Expand Down Expand Up @@ -458,7 +459,7 @@ describe(__filename, () => {
sinon.assert.calledWith(
dispatchSpy,
deleteAddonReview({
addonId: fakeAddon.id,
addonId: review.reviewAddon.id,
errorHandlerId: errorHandler.id,
reviewId: review.id,
}),
Expand Down Expand Up @@ -1273,7 +1274,7 @@ describe(__filename, () => {
sinon.assert.calledWith(
dispatchSpy,
deleteAddonReview({
addonId: fakeAddon.id,
addonId: review.reviewAddon.id,
errorHandlerId: errorHandler.id,
reviewId: review.id,
isReplyToReviewId: originalReviewId,
Expand All @@ -1292,6 +1293,22 @@ describe(__filename, () => {
const reviewComponent = root.find(UserReview);
expect(reviewComponent).toHaveProp('showRating', false);
});

it('configures DismissibleTextForm with an ID', () => {
const { review } = _setReviewReply();
store.dispatch(showReplyToReviewForm({ reviewId: review.id }));

const root = render({ review });

const form = root.find(DismissibleTextForm);

expect(form).toHaveProp('id');
const formId = form.prop('id');

expect(formId).toContain('AddonReviewCard');
expect(formId).toContain(`addon-${review.reviewAddon.id}`);
expect(formId).toContain(`review-${review.id}`);
});
});

describe('siteUserCanManageReplies', () => {
Expand Down
Loading