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.

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

Feat: Reuse Canvas Assets #25764

Merged
merged 15 commits into from Dec 4, 2018

Conversation

@cqliu1
Copy link
Contributor

commented Nov 15, 2018

Close #25533.

This adds the ability to reuse assets without editing an element's expression.

Changes:

  • adds Create image element button on each asset in the asset manager
  • adds an asset picker for selecting existing assets as part of the image upload form
  • switches image upload type selection from select control to button group
  • removes image preview from filepicker view
  • add tooltips to icon buttons in asset manager

Asset Manager

Users can create an image element using any of the assets in the asset manager.
nov-15-2018 15-34-17

Image Upload

Users can also select from existing assets in the sidebar form.
nov-15-2018 15-32-32

Empty state (no assets)

screen shot 2018-11-15 at 3 18 45 pm

With existing assets

screen shot 2018-11-15 at 3 24 31 pm

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

@w33ble
Copy link
Contributor

left a comment

This functionality is great! Still looking this over, but I had some initial comments on my quick pass through the code.

if (urlTypeInline) {
uploadImage = loading ? (
const forms = {
file: loading ? (

This comment has been minimized.

Copy link
@w33ble

w33ble Nov 16, 2018

Contributor

nit: this component is getting pretty large now, having 3 different "image upload form" components in that folder instead, and using them here, might be a nice change.

type: 'function',
function: 'asset',
arguments: {
_: [asset.id],

This comment has been minimized.

Copy link
@w33ble

w33ble Nov 16, 2018

Contributor

nit: since you're only using the asset's ID here, it would be nice of the handler just took the ID, like copyAsset and removeAsset does (as assetId).

<EuiFlexItem className="asset-create-image" grow={false}>
<EuiToolTip content="Create image element">
<EuiButtonIcon
iconType="plusInCircle"

This comment has been minimized.

Copy link
@w33ble

w33ble Nov 16, 2018

Contributor

We use vector for the "Add element" control, I think it would make sense, and probably be more consistent, to do the same here.

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

I'm getting errors if I have assets and I add a new image.

screenshot 2018-11-16 14 39 15

I also see a bunch of proptype errors, which are almost certainly caused by that error. But, just in case they are helpful:

screenshot 2018-11-16 14 39 42

screenshot 2018-11-16 14 39 35


UPDATE: two different ways to repro...

  1. If you already have assets in your workpad, adding a new image element will cause this.
  2. If you don't have any assets, adding a new image will work, but you'll get the same set of errors if you import/upload an image.

UPDATE2: third repro...

  1. Edit the expression and set the asset id to something invalid.

I think it might happen any time selectedAsset is undefined/null.

@w33ble
Copy link
Contributor

left a comment

Errors need fixin'

if (assets.length) {
urlTypeOptions.unshift({ id: 'asset', label: 'Asset' });
selectedAsset = assets.find(({ value }) => value === url);
console.log({ selectedAsset });

This comment has been minimized.

Copy link
@w33ble

w33ble Nov 16, 2018

Contributor

Remove this.

@cqliu1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@w33ble I made all the changes you requested. This is ready for another look

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2018

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Still getting this error when I try adding an image element, or even select an existing one, when I have no assets uploaded:

screenshot 2018-11-21 10 55 34

@cqliu1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

@w33ble The bug is fixed for sure this time.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

I spent most of the day dealing with the CI, so I haven't had a chance to look at this again. If the error went away, I'd probably approve it though. If someone else can confirm that it all works as expected, you don't need to wait for me, especially since I'm out for the next week.

@alexfrancoeur
Copy link
Contributor

left a comment

Just tried this out, LGTM 👍 Thanks Catherine, this is huge!

cqliu1 added 3 commits Nov 28, 2018
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

@w33ble
w33ble approved these changes Nov 28, 2018
Copy link
Contributor

left a comment

This is looking good!

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

I did fine one oddity, but it seems like an edge case we can address later. If the image I'm using is a dataurl, but the content matches an asset, the sidebar will make it look like I'm using an asset.

screenshot 2018-11-28 13 17 32

This is pretty easy to replicate, since you'll get the dataurl in the Link section automatically, and it'll use the resolved value of the asset.

  1. Add image element
  2. Upload image
  3. Switch to Link once it uploads and shows the asset in the sidebar
  4. Click the "Set" button
  5. Note that the interface switches back to assets and makes it look like it's using the asset directly

There's two issues actually; it switches to the assets view when you click set, and it looks like you're using an asset even though you are not.

I'm fine leaving it this way though, and I'm not sure we can actually fix it...

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

@cqliu1 another thing I noticed, the order of the assets changes, which ends up being pretty frustrating:

nov-28-2018 13-24-18

It's especially disorienting when you have a lot of assets:

nov-28-2018 13-29-54

I don't want to block your PR on it, but if it's an easy thing to change, I think it would be a much better experience if the order was fixed. If you want to merge this PR as-is, please open another issue about that behavior.

@cqliu1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

@w33ble I reorder the assets and bring the selected asset to the top because anytime you choose as asset, it causes a rerender of the sidebar, and the asset_picker scrolls all the way to the top. It's a quick fix to remove the reordering, but I thought it was as even more awkward experience to have the picker scroll to reset to the top without being able to see was selected.

nov-28-2018 15-24-20

I think ideally I'd like to preserve the position of the scroll or scroll to the selected asset so it's in view, which can definitely be done in a separate PR later on to improve the form.

@w33ble

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

That's a good point. I wonder if we can show the selected asset differently, via duplication or something. Anyway, like I said, I don't want to block this PR on that behavior, feel free to just merge this and open another issue and we can deal with it later.

@cqliu1

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@w33ble It actually wasn't too difficult to implement. Now it no longer changes the asset order and scrolls to the selected asset. No need to create a separate issue.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

@rashidkpc

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Sounds like this is GTG then? 🎉

@@ -24,7 +25,7 @@ export const ArgForm = compose(
}
},
}),
connect(state => ({ workpad: getWorkpadInfo(state) }))
connect(state => ({ workpad: getWorkpadInfo(state), assets: getAssets(state) }))

This comment has been minimized.

Copy link
@w33ble

w33ble Dec 3, 2018

Contributor

nit: I realize this was already like this, but hoisting it to mapStateToProps like in other containers would better fit convention.


componentDidMount() {
const selectedAsset = document.getElementById('canvasAssetPicker__selectedAsset');
if (selectedAsset) selectedAsset.scrollIntoView();

This comment has been minimized.

Copy link
@w33ble

w33ble Dec 3, 2018

Contributor

I don't think this is quite right, MDN says the following:

The Element.scrollIntoView() method scrolls the element on which it's called into the visible area of the browser window.

I think this means that it'll scroll the entire document body to get the element into view, not just its scrollable container. I may be wrong, but with the currently broken page preview layout in master, I end up with this view once I pick an asset:

dec-03-2018 12-42-28

Normally this would still look right, since the size of the application is "fixed", but isolating the scroll action to the asset container would be safer.

This comment has been minimized.

Copy link
@w33ble

w33ble Dec 3, 2018

Contributor

That said, it's probably fine for this PR, since it only behaves weird because the layout is currently broken. It can be changed later if needed.

This comment has been minimized.

Copy link
@cqliu1

cqliu1 Dec 3, 2018

Author Contributor

I've got a fix for the page preview bug #26573, so it shouldn't be an issue once both are merged.

I would've prefered to use Element.scrollTo() which in theory would isolate the scroll to the asset picker, but it's not supported in IE11 whereas there is basic support for Element.scrollIntoView() in IE11.

We can explore other ways of preserving the scroll if it presents to be a problem later on.

This comment has been minimized.

Copy link
@w33ble

w33ble Dec 4, 2018

Contributor

not supported in IE11

😢

We can explore other ways of preserving the scroll if it presents to be a problem later on.

👍

@elastic elastic deleted a comment from elasticmachine Dec 3, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

@cqliu1 cqliu1 merged commit cf0da36 into elastic:master Dec 4, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
kibana-ci Build finished.
Details

@cqliu1 cqliu1 deleted the cqliu1:feat/reuse-assets branch Dec 4, 2018

cqliu1 added a commit to cqliu1/kibana that referenced this pull request Dec 4, 2018
Feat: Reuse Canvas Assets (elastic#25764)
* Added asset picker to image upload arg form

* Added button to create image element in asset manager

* Added hover style

* Extracted image upload forms into separate components

* Fixed new image element with no assets bug

* Fixed import

* Removed unused styles

* Fixed import order

* Scrolls to selected asset

* Fixed asset selector
cqliu1 added a commit that referenced this pull request Dec 4, 2018
Feat: Reuse Canvas Assets (#25764) (#26644)
* Added asset picker to image upload arg form

* Added button to create image element in asset manager

* Added hover style

* Extracted image upload forms into separate components

* Fixed new image element with no assets bug

* Fixed import

* Removed unused styles

* Fixed import order

* Scrolls to selected asset

* Fixed asset selector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.