Skip to content

Conversation

@solocommand
Copy link
Member

@solocommand solocommand commented Jan 17, 2019

Support pausing for line items and ads

@solocommand solocommand changed the title Support Pausing line items Support Pausing Jan 18, 2019
@solocommand
Copy link
Member Author

screen shot 2019-01-22 at 10 21 53 am

screen shot 2019-01-22 at 10 21 38 am

*
* @param {Event} event
*/
debounceInput(event) {
Copy link
Member

Choose a reason for hiding this comment

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

Debouncing of input isn't needed

*
* @param {Event} event
*/
debounceChange(event) {
Copy link
Member

Choose a reason for hiding this comment

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

Debouncing of change isn't needed

export default Controller.extend(ActionMixin, ObjectQueryManager, {
isActive: computed.not('model.paused'),
isPausedDisabled: computed('isActionRunning', 'model.status', function() {
if (this.get('isActionRunning')) return true;
Copy link
Member

Choose a reason for hiding this comment

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

Disabling of the switch should just use disabled={{isActionRunning}} in the template... or, rather, the autosave switch component should disable itself when running. Then you do not need to use this

/**
*
*/
async pause() {
Copy link
Member

Choose a reason for hiding this comment

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

This should take the value directly as an argument, and not use the bound value

import pauseLineItem from '@base-cms/parcel-plug-manage/gql/mutations/lineitem/pause';

export default Controller.extend(ObjectQueryManager, ActionMixin, {
isActive: computed.not('model.paused'),
Copy link
Member

Choose a reason for hiding this comment

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

There is not an isActive() method on the model schema... perhaps make this a graph field and use instead of a computed property

/**
*
*/
async pause() {
Copy link
Member

Choose a reason for hiding this comment

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

Should use the value directly as an arg, instead of the mode value

* The number of milliseconds to debounce the
* change event by when typing.
*/
typeDelay: 750,
Copy link
Member

Choose a reason for hiding this comment

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

Type delay can be removed

try {
await this.get('apollo').mutate({ mutation, variables }, 'pauseAd');
} catch (e) {
this.get('graphErrors').show(e);
Copy link
Member

Choose a reason for hiding this comment

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

This should throw the error so the form component can handle it as a validation error message

try {
await this.get('apollo').mutate({ mutation, variables }, 'pauseLineItem');
} catch (e) {
this.get('graphErrors').show(e);
Copy link
Member

Choose a reason for hiding this comment

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

This should throw the error so the form component can handle it as a validation error message

this.get('graphErrors').show(e);
} finally {
this.endAction();
this.set('isPausing', false);
Copy link
Member

Choose a reason for hiding this comment

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

The isPausing would not be necessary if you make the switch auto save component disable itself on save

@zarathustra323
Copy link
Member

@solocommand I know it's annoying, but can you manually add a class to the switch and add custom css to make it the same height/width of the regular input elements?

*
* @param {Event} event
*/
debounceChange({ target }) {
Copy link
Member

Choose a reason for hiding this comment

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

Debounce change can be removed still :)

<div class="custom-control custom-switch">
{{yield
(hash
element=(component "form-elements/input"
Copy link
Member

Choose a reason for hiding this comment

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

Should this use the form-elements/checkbox component?


validator=validator
on-insert=(action "setChildInputComponent")
change=(action "debounceChange")
Copy link
Member

Choose a reason for hiding this comment

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

Change can fire the sendOnChange directly

@zarathustra323
Copy link
Member

@solocommand I added a few more comments yesterday... just wanted to check if still had any changes to push, or if I should just merge?

@zarathustra323 zarathustra323 merged commit 2e83389 into base-cms:master Jan 23, 2019
@solocommand solocommand deleted the paused branch January 23, 2019 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants