Skip to content

Support All Asset Types for Media Widget #437

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

Merged
merged 3 commits into from
Jan 6, 2017

Conversation

toastercup
Copy link
Member

Add supporting infrastructure to allow Media Widget to support all asset types, not just images

@@ -6,7 +6,7 @@
= cell('index/content_item', nil, { cells: column[:cells], content_item: content_item }).(:column)
- if context[:popup]
%td{ class: 'mdl-data-table__cell--non-numeric' }
%a{ href: "#", data: { id: content_item.id, title: content_item_title(content_item), thumb: content_item_thumb_url(content_item), src: content_item_asset_url(content_item), alt: content_item_asset_alt_text(content_item) }, class: "media-select--#{context[:popup]} mdl-button mdl-js-button mdl-button--icon" }
%a{ href: "#", data: { id: content_item.id, title: content_item_title(content_item), thumb: content_item_thumb_url(content_item), url: content_item_asset_url(content_item), alt: content_item_asset_alt_text(content_item), asset_type: content_item_asset_type(content_item) }, class: "media-select--#{context[:popup]} mdl-button mdl-js-button mdl-button--icon" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume I'm over looking it, but where does the URL get fed into a src attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens in cortex-plugins-core, here: https://github.com/cortex-cms/cortex-plugins-core/pull/39/files#diff-f5165bb3219f906c9714100622beb28eR18

The way things are happening right now are needlessly complex. In our React rebuild, this should simplify significantly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep I just got to that part!

Copy link
Contributor

Choose a reason for hiding this comment

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

So if someone embeds a doc file, how would that be presented online when the blog post is viewed?

Copy link
Member Author

Choose a reason for hiding this comment

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

As a link

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if it was a PDF would they want it to be a PDF view?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah - almost always, they just include PDFs as links. If they want a PDF view, they'll come to us with that requirement. Keep in mind this is for a blog - I can't think of many blogs that embed PDFs in their copy

Copy link
Member Author

Choose a reason for hiding this comment

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

AKA: Usually PDFs are for 'take home' materials - things you want the user to download so they can view later or print to share with others

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok just checking.

Copy link
Contributor

@MKwenhua MKwenhua left a comment

Choose a reason for hiding this comment

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

👍

@toastercup toastercup merged commit 6c27459 into develop Jan 6, 2017
@toastercup toastercup deleted the media-widget-support-all-asset-types branch January 6, 2017 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants