Adds asset file upload #11450

javierarce · 2017-02-03T15:21:27Z

This PR enables the upload of assets using direct file upload (of single or multiple files) and also revamps the asset modal window adding tabs.

In addition of those changes, whenever a user uploads a custom asset, the input field of the Fill components shows a generic icon to indicate an image is being used as a marker.

How to do acceptance

Enable the icon-styling flag.
Create a map with some points
Style them however you want

Select an asset from the quick list in the Fill component
Select an asset from the modal window
Upload an asset
Delete/Select All/Deselect all custom assets from the modal window
Upload assets (with a URL or using the file browser)

xavijam · 2017-02-06T11:12:12Z

Hey, has any designer reviewed this @javierarce?

xavijam · 2017-02-06T11:39:20Z

I can't comment it in the code, but maki icons, simple icons and pin icons disclaimers are not translated, appear in plain english. Do we agree with that?

xavijam

Basically small details, I'd like to pass the acceptance as well. Things that worry me:

One error state that only displays a console, let's add the error view please.
CSS discussion about keep using the old styles or break with the past and look to the future (@piensaenpixel).

xavijam · 2017-02-06T11:09:01Z

lib/assets/core/javascripts/cartodb3/components/error/error-view.js

@@ -21,6 +21,7 @@ module.exports = CoreView.extend({
        desc: _t('components.error.default-desc')
      }
    );
+    this._template = this.options && this.options.template || baseTemplate;


this.options is always available under a Backbone View, so we don't need the first check.

I got an undefined error on the specs when I removed that check, I'm not sure why is not defined in this case :|

We reviewed this together and found that this.options is not defined.

xavijam · 2017-02-06T11:15:17Z

...todb3/components/form-components/editors/fill/input-color/assets-picker/asset-header-view.js

+  initialize: function (opts) {
+    checkAndBuildOpts(opts, REQUIRED_OPTS, this);
+
+    this._assets = this.options.assets;


Is it a collection? If so, I'd call it _assetsCollection, in order to be more specific.

Or if it is an array, _assetsArray.

xavijam · 2017-02-06T11:16:48Z

...todb3/components/form-components/editors/fill/input-color/assets-picker/asset-header-view.js

+  },
+
+  _initBinds: function () {
+    this._assets.bind('add change remove', this.render, this);


Let's try to use listenTo if possible (http://stackoverflow.com/questions/16823746/backbone-js-listento-vs-on). If not remember we need to add our add_related_model thing.

xavijam · 2017-02-06T11:17:22Z

...todb3/components/form-components/editors/fill/input-color/assets-picker/asset-header-view.js

+      editable: this._editable,
+      assetsCount: this._assets.length,
+      selectedCount: selectedCount,
+      allSelected: selectedCount === this._assets.length


If assets is a collection, size method is available.

xavijam · 2017-02-06T11:20:31Z

...artodb3/components/form-components/editors/fill/input-color/assets-picker/asset-item-view.js

+
+  _onClick: function (e) {
+    this.killEvent(e);
+    this.trigger('selected', this.model);


Couldn't we set an attribute called selected within the model? With that, we could check the collection changes instead of passing a custom event to the parent.

Or better, I'm checking that we control that with the state attribute. Do we need this event then?

I agree that it a regular case, it would make total sense to use the model state. The reason for my decision is that changing the state of an asset in this particular case doesn't only imply the asset itself, but the state of other assets (and this changes if the shift key is pressed). In this case it made more sense to centralice the selection in a common place instead of having it separated in two different locations (view of the item and parent view)

xavijam · 2017-02-06T11:55:38Z

...3/components/form-components/editors/fill/input-color/assets-picker/user-assets-list-view.js

+    }
+  },
+
+  _isLoading: function () {


I've seen several times this function, shouldn't we move it within the proper model? Like a helper one. Same for the below one.

Refactored the upload-tab view to use the upload method from the main modal (assets-view.js)

xavijam · 2017-02-06T11:56:31Z

...3/components/form-components/editors/fill/input-color/assets-picker/user-assets-list-view.js

+    }, this);
+
+    _.each(selectedAssets, function (asset) {
+      asset.destroy({


I'd not remove any asset until we have a response from the backend. Could we use here {wait: true} within the backbone destroy method.

Yeah, I tried this at first, but I couldn't make it work using wait :/

Ok, talked with you and decided that the first approach was ok for this case (we wait until the deletion is complete and show the list again)

xavijam · 2017-02-06T11:58:18Z

...artodb3/components/form-components/editors/fill/input-color/assets-picker/user-assets-tab.js

+  },
+
+  _onFetchAssetsError: function () {
+    console.log('Error');


We should render an error message, not only a console.

xavijam · 2017-02-06T12:00:21Z

...rtodb3/components/form-components/editors/fill/input-color/input-color-fixed-content-view.js

@@ -62,6 +62,7 @@ module.exports = CoreView.extend({
      name: 'file',
      type: this.model.get('image') ? 'image' : 'text',
      label: this.model.get('image') ? this.model.get('image') : _t('form-components.editors.fill.input-color.img'),
+      kind: this.model.get('kind') ? this.model.get('kind') : 'marker',


this.model.get('kind') || 'marker' ?

xavijam · 2017-02-06T12:01:07Z

...core/javascripts/cartodb3/components/form-components/editors/fill/input-color/input-color.js

@@ -95,8 +96,16 @@ module.exports = CoreView.extend({
    return false;
  },

+  _activatedFlag: function () {
+    return this._userModel && this._userModel.featureEnabled('icon-styling');


I feel @matallo did something similar or I'm suffering a Dejavù??.

indeed! some lines below

cartodb/lib/assets/core/javascripts/cartodb3/components/form-components/editors/fill/input-color/input-color.js

Line 111 in 7682205

_iconStylingEnabled: function () {

it also checks wether _imageEnabled (this comes from editorAttrs at editor creation) as only points geometry have image enabled. I'm not sure if this applies here, too, but in any case the this._userModel && this._userModel.featureEnabled('icon-styling') part can be put in just one place

javierarce · 2017-02-06T12:12:28Z

Hey, has any designer reviewed this @javierarce?

Not in staging, @xavijam. I'll deploy it somewhere so they can see it.

xavijam · 2017-02-07T16:26:47Z

...core/javascripts/cartodb3/components/form-components/editors/fill/input-color/input-color.js

@@ -96,20 +96,16 @@ module.exports = CoreView.extend({
    return false;
  },

-  _activatedFlag: function () {


isn't it necessary anymore?

xavijam · 2017-02-07T16:27:13Z

...artodb3/components/form-components/editors/fill/input-color/assets-picker/user-assets-tab.js

-  _onFetchAssetsError: function () {
-    console.log('Error');
+  _onFetchAssetsError: function (model, response) {
+    this.trigger('error', response);


xavijam

Let's go to acceptance!

javitonino · 2017-02-08T08:31:00Z

A couple of questions while I am still testing:

If I style using a custom uploaded icon, the fill component does not show the select icon in the layer view. Intended?
Should this supports organization assets? They are not shown in the builder view now that it has tabs.

xavijam · 2017-02-08T08:33:19Z

Yes, intended, it is ok.
Ticket is already created -> Enable the possibility to select an icon previously uploaded by the organization admin #11462.

javitonino · 2017-02-08T08:53:00Z

I have tested:

Selecting icons from all pages in the icon modal
Uploading files from file and URL
Deleting files
Select and deselect all

Everything works awesome 👏

Only a couple design issues (designer hat on), at least on Firefox:

Alignment changes by one pixel when the actions (select all, delete) in the upload tab are shown.

To reproduce, deselect all, stare at your screen, and select one icon in the "Your uploads" tab. You should see the icons move down a pixel.
The actions are not consistent with other parts of the interface:

The Select/Deselect text is black (should be blue)
Both links are not underlined when hovered over
No "X items selected" (this probably doesn't make too much sense here)

For reference:
In org assets:

In builder modal:

They are minor, but I think they are legit.

xavijam · 2017-02-08T08:55:50Z

Great, thanks for discovering that @javitonino. @urbanphes could you check those? Because you checked the feature in staging (in the name of the design team 💃).

javierarce · 2017-02-08T09:33:34Z

Agree with the style of the link, sorry about that.

The number of selected items didn't appear in the design, so I'd handle that in a different ticket. And for the 1px issue, I'd open a new ticket.

To speed up the release of this feature, I'll take it and add the required class to fix the link style.

Thanks for the thorough review!

javierarce · 2017-02-08T09:45:10Z

Styling classes added.

javierarce · 2017-02-08T09:47:15Z

@javitonino did you tried enabling/disabling the feature flag?

javitonino · 2017-02-08T09:51:51Z

Yep, there is only a small issue that if you activate it, apply a style with icons, deactivate the FF, the map keeps the icons but you cannot reset them from the style properties. But it is minor (you can reset using cartocss) and only when "downrgrading", so 👍

Everything is hidden when the FF is disabled, and everything works with it enabled ✨

javierarce · 2017-02-08T09:53:50Z

Great, thanks! Promoting it to the next stage.

javierarce added 26 commits February 1, 2017 17:34

Aligns asset

aa8890f

Allow adding a custom template for the error view

126d619

Allows selecting item by name

c34ffee

Stores kind of the selected asset

77d0bb4

Adds file upload for assets

a01327a

Fixes header binding

34d21ad

Adds opts check

72c9899

Removes requirements

6165e59

Fixes selected image

3692bc4

Hides disclaimers

03c0be7

Removes TODO

fbfde21

Add specs

497072c

Adds hover

81227f0

Minimal refactor

681b189

Adds more specs

2b60740

Little refactor

eb19874

Adds cleanup

35982b2

More specs

f35bb40

Adds specs and fixes url upload

e3186c0

Adds letter capitalization

5fa9a71

Merge with master

7f1b82f

Removes duped file

0a16069

Rename method

c15238b

Revert change

11f9ec2

Fixes semistandard warnings

0504c3f

Removes redundant code

7682205

xavijam reviewed Feb 6, 2017

View reviewed changes

javierarce added 14 commits February 7, 2017 16:17

uses new name

9008052

Refactor

6a21f85

Triggers error

e5b7762

refactor

9666dd5

Adds useful methods. Uses option checker

1deac5b

Uses unset

8b44620

Uses el instead of

b4a6467

uses unset

e77b404

refactor

b18df38

refactor

dbf957e

Refactor

be37c4e

Fixes test

b1fcd3f

Fixes spelling error

d4ce0f2

Merge branch 'master' into 11020-file-upload

d47a2c6

xavijam reviewed Feb 7, 2017

View reviewed changes

xavijam approved these changes Feb 7, 2017

View reviewed changes

Bump package

5422792

Add missing styling classes

a8e0d3e

javitonino merged commit d97accb into master Feb 8, 2017

javitonino deleted the 11020-file-upload branch February 8, 2017 09:55

Adds asset file upload #11450

Adds asset file upload #11450

Conversation

javierarce commented Feb 3, 2017 • edited by matallo

How to do acceptance

xavijam commented Feb 6, 2017 • edited

xavijam commented Feb 6, 2017 • edited

xavijam left a 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

javierarce Feb 7, 2017 • edited

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

javierarce commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavijam left a comment

Choose a reason for hiding this comment

javitonino commented Feb 8, 2017

xavijam commented Feb 8, 2017

javitonino commented Feb 8, 2017 • edited

xavijam commented Feb 8, 2017 • edited

javierarce commented Feb 8, 2017 • edited

javierarce commented Feb 8, 2017

javierarce commented Feb 8, 2017

javitonino commented Feb 8, 2017

javierarce commented Feb 8, 2017

javierarce commented Feb 3, 2017 •

edited by matallo

xavijam commented Feb 6, 2017 •

edited

xavijam commented Feb 6, 2017 •

edited

javierarce Feb 7, 2017 •

edited

javitonino commented Feb 8, 2017 •

edited

xavijam commented Feb 8, 2017 •

edited

javierarce commented Feb 8, 2017 •

edited