FIX Create campaign success message #37

phalkunz · 2017-07-21T02:09:56Z

Issue: #16

Requires silverstripe/silverstripe-admin#178 to be merged first

Merge these after this

Enhancement add files as a placeholder for an empty campaign silverstripe-asset-admin#573
Enhancement add pages as a placeholder for an empty campaign silverstripe-cms#1918

flamerohr · 2017-07-26T23:27:21Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

@@ -352,6 +436,7 @@ function mapStateToProps(state, ownProps) {
    record: record || {},
    campaign: state.campaign,
    treeClass,
+    newItemCreated: state.campaign.newItemCreated,


proptypes definition for this

flamerohr · 2017-07-26T23:27:50Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

-    if (this.props.record.State === 'open') {
+    if (!items || items.length === 0) {
+      actionProps = Object.assign(actionProps, {
+        title: i18n._t('CampaignAdmin.PUBLISHCAMPAIGN'),


add translation default fallback

flamerohr · 2017-07-26T23:28:53Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

+          className="flexbox-area-grow
+          fill-height
+          preview
+          campaign-admin__campaign-preview


shouldn't have new lines in double quoted strings...
move these to a variable or classnames object

flamerohr · 2017-07-26T23:30:58Z

client/src/state/campaign/CampaignReducer.js

@@ -6,6 +6,7 @@ const initialState = deepFreeze({
  changeSetItemId: null,
  isPublishing: false,
  view: null,
+  newItemCreated: false,


the name for this is quite confusing, since this is a boolean type, it should describe the question like isNewItem or hasNewItem.
newItemCreated implies it's an object that was just created.

flamerohr · 2017-07-26T23:34:31Z

client/src/state/campaign/CampaignActions.js

+ * @param {boolean}
+ * @return {Object}
+ */
+export function setNewCampaignCreated(newItemCreated) {


naming is inconsistent with what it's doing, maybe setIsNewItem or markHasCreatedItem

having both New and Created is double up on meaning.

flamerohr · 2017-07-26T23:36:35Z

client/src/containers/CampaignAdmin/CampaignAdmin.js

@@ -119,7 +120,8 @@ class CampaignAdmin extends SilverStripeComponent {
          // open the new campaign in edit mode after save completes
          const sectionUrl = this.props.sectionConfig.url;
          const id = response.record.id;
-          this.props.router.push(`${sectionUrl}/set/${id}/edit`);
+          this.props.campaignActions.setNewCampaignCreated(true);


Since the id is available, it would be more extensible to include the record or even just the id in the record, rather than just a boolean.
In this case, you could ignore the is or has prefix I mentioned below. But try to keep the naming consistent still setNewItem is perfectly fine.

tractorcow · 2017-08-02T04:55:12Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

-      );
+    const newItemInfo = newItem ?
+      (<p className="alert alert-success alert--no-border" role="alert">
+        Nice one! You have successfully created a campaign.


i18n this please.

tractorcow · 2017-08-02T04:55:22Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

+        <div className={previewClasses}>
+          <h2 className="campaign-admin__empty-heading">Getting started</h2>
+          <p className="campaign-admin__empty-info">
+            Select <strong>Add to Campaign</strong> from pages, files, and other content types


i18n this please

be careful with adding html to translation strings, it's heavily discouraged because you need to use dangerouslySetInnerHTML in react... (or castStringToElement())
Can we use quotes instead?
I've raised this about the "Nice one!" being bold above as well, and it was agreed that we could remove the bold for that.

flamerohr · 2017-08-02T22:22:13Z

client/src/state/campaign/CampaignActions.js

+/**
+ * Set new campaign
+ *
+ * @param {?number}


invalid JSDoc @param {Number|null}

flamerohr · 2017-08-02T22:37:01Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

@@ -29,6 +28,16 @@ class CampaignAdminList extends SilverStripeComponent {
    this.handleItemSelected = this.handleItemSelected.bind(this);
    this.setBreadcrumbs = this.setBreadcrumbs.bind(this);
    this.handleCloseItem = this.handleCloseItem.bind(this);
+
+    if (!this.isRecordLoaded()) {


since this is in the constructor, would be a good idea to pass through props to isRecordLoaded it can default to this.props if not provided

flamerohr · 2017-08-02T23:41:38Z

src/CampaignAdmin.php

@@ -550,7 +590,7 @@ public function getCampaignCreateForm()
            'campaignCreateForm',
            $fields,
            FieldList::create(
-                FormAction::create('save', _t(__CLASS__.'.SAVE', 'Save'))
+                FormAction::create('save', _t(__CLASS__.'.SAVE', 'Create'))


this isn't good practice, please create a new translation key entry... the fallback doesn't even take effect.

flamerohr · 2017-08-03T00:28:30Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

+        <div className={previewClasses}>
+          <h2 className="campaign-admin__empty-heading">Getting started</h2>
+          <p className="campaign-admin__empty-info">
+            Select <strong>Add to Campaign</strong> from pages, files, and other content types


be careful with adding html to translation strings, it's heavily discouraged because you need to use dangerouslySetInnerHTML in react... (or castStringToElement())
Can we use quotes instead?
I've raised this about the "Nice one!" being bold above as well, and it was agreed that we could remove the bold for that.

flamerohr · 2017-08-03T00:34:35Z

src/CampaignAdmin.php

+                'noItemText' => 'Add from ' . $class->i18n_plural_name() . ' area',
+                'items' => []
+            ];
+        }


I'm not comfortable with the idea of cross-module references like this...
Can these be added by an extension hook, rather than hardcoded in to campaign admin?
It could even be an array of classes in a config.

Either extension hook or config will be fine, both could be a best of both worlds.

flamerohr · 2017-08-03T00:38:32Z

src/CampaignAdmin.php

+                'baseClass' => DataObject::getSchema()->baseDataClass('SilverStripe\Assets\File'),
+                'singular' => $class->i18n_singular_name(),
+                'plural' => $class->i18n_plural_name(),
+                'noItemText' => 'Add from ' . $class->i18n_plural_name() . ' area',


Oops and this could be i18n translated

…der groups

flamerohr · 2017-08-03T01:28:29Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

+            {
+              listGroupItems.length > 0
+                ? listGroupItems
+                : <p className="list-group-item">{group.noItemsText}</p>


@phalkunz please note, we're following airbnb formatting standards (to an extent) so ternary conditions should break like this:

condition ? item : null

ref: https://github.com/airbnb/javascript/#comparison--nested-ternaries the better section

tractorcow · 2017-08-03T02:01:35Z

src/CampaignAdmin.php

@@ -155,6 +159,34 @@ public function readCampaigns()
        return $response;
    }

+    protected function getPlaceholderGroups()


PHPDoc please.

tractorcow · 2017-08-03T02:03:57Z

src/CampaignAdmin.php

+    {
+        $groups = [];
+
+        $classes = Config::inst()->get(self::class, 'placeholder_group_classes');


This is a duplication of ChangeSet::important_classes config.

What do you mean by this? Should we be using ChangeSet.important_classes instead?

Never mind, I see your comment below.

tractorcow · 2017-08-03T02:16:52Z

client/src/containers/CampaignAdmin/CampaignAdminList.js

@@ -315,6 +374,19 @@ class CampaignAdminList extends SilverStripeComponent {
    return groups;
  }

+  getPlaceholderGroups() {


I'm not sure why we need this api; doesn't groupItemsForSet() build the groups automatically?

ok, @phalkunz has explained why this is necessary. The only change I'd request is the switch to using the existing config for important_classes instead.

phalkunz · 2017-08-03T02:32:37Z

src/CampaignAdmin.php

+                        'data' => [
+                            'pristineTitle' => _t(__CLASS__.'SAVED', 'Saved'),
+                            'pristineIcon' => 'tick',
+                            'dirtyTitle' => _t(__CLASS__.'SAVECHANGES', 'Save changes'),


@unclecheese Looks like this render the title set above (line 521) redundant.

tractorcow · 2017-08-03T04:54:41Z

src/CampaignAdmin.php

+
+        foreach ($classes as $class) {
+            /** @var DataObject $item */
+            $item = Injector::inst()->get($class);


get() won't return null if the class doesn't exist, it'll throw an exception. Can you use class_exists() instead please?

All feedback addressed :)

phalkunz mentioned this pull request Jul 21, 2017

On successful submission of create campaign form, there's no success message #16

Closed

11 tasks

phalkunz changed the title ~~Change "Add campaign" to "Add new"~~ FIX Create campaign success message Jul 21, 2017

Change button labels

ec321c7

phalkunz force-pushed the pulls/1.0/create-success-msg branch from 0840b53 to ec321c7 Compare July 21, 2017 02:16

phalkunz added 4 commits July 24, 2017 10:26

Add message for newly created campaign

245b9ad

Empty and loading preview

f13a96c

Add title, icon, and class for Save button in saved form version

7ef933b

Add placeholder groups

82ec3f4

flamerohr suggested changes Jul 26, 2017

View reviewed changes

phalkunz force-pushed the pulls/1.0/create-success-msg branch from 7af190b to 280d637 Compare July 27, 2017 04:51

Clean up

65e66c2

phalkunz force-pushed the pulls/1.0/create-success-msg branch from 280d637 to 65e66c2 Compare July 27, 2017 23:35

Aaron Carlino added 2 commits August 1, 2017 17:19

Update to use simple props, without new FormAction API

4162b88

Build

12b80f0

unclecheese assigned flamerohr Aug 2, 2017

Add i18n for pristine/dirty titles

4a8b2c0

tractorcow suggested changes Aug 2, 2017

View reviewed changes

flamerohr suggested changes Aug 3, 2017

View reviewed changes

Fix some of the feedback addressed

32edfa0

flamerohr suggested changes Aug 3, 2017

View reviewed changes

flamerohr reviewed Aug 3, 2017

View reviewed changes

Christopher Joe added 2 commits August 3, 2017 13:15

Fix move placeholders to config and added extension hook for placehol…

ccdda08

…der groups

Fix translations in list

1d4e524

flamerohr reviewed Aug 3, 2017

View reviewed changes

This was referenced Aug 3, 2017

Enhancement add pages as a placeholder for an empty campaign silverstripe/silverstripe-cms#1918

Closed

Enhancement add files as a placeholder for an empty campaign silverstripe/silverstripe-asset-admin#573

Closed

tractorcow suggested changes Aug 3, 2017

View reviewed changes

phalkunz commented Aug 3, 2017

View reviewed changes

Minor fixes per Chris/Damian feedback

e223da7

flamerohr approved these changes Aug 3, 2017

View reviewed changes

tractorcow previously requested changes Aug 3, 2017

View reviewed changes

Fix class check

2828216

flamerohr merged commit fe5768f into silverstripe:1 Aug 3, 2017

flamerohr deleted the pulls/1.0/create-success-msg branch August 3, 2017 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Create campaign success message #37

FIX Create campaign success message #37

phalkunz commented Jul 21, 2017 •

edited by flamerohr

Loading

flamerohr Jul 26, 2017

flamerohr Jul 26, 2017

flamerohr Jul 26, 2017

flamerohr Jul 26, 2017

flamerohr Jul 26, 2017

flamerohr Jul 26, 2017

tractorcow Aug 2, 2017

tractorcow Aug 2, 2017

flamerohr Aug 3, 2017

flamerohr Aug 2, 2017

flamerohr Aug 2, 2017

flamerohr Aug 2, 2017

flamerohr Aug 3, 2017

flamerohr Aug 3, 2017

flamerohr Aug 3, 2017

flamerohr Aug 3, 2017

phalkunz Aug 3, 2017

tractorcow Aug 3, 2017

tractorcow Aug 3, 2017

unclecheese Aug 3, 2017

unclecheese Aug 3, 2017

tractorcow Aug 3, 2017

tractorcow Aug 3, 2017

phalkunz Aug 3, 2017

tractorcow Aug 3, 2017

FIX Create campaign success message #37

FIX Create campaign success message #37

Conversation

phalkunz commented Jul 21, 2017 • edited by flamerohr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phalkunz commented Jul 21, 2017 •

edited by flamerohr

Loading