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

media_folder on widged level #492

Closed
wants to merge 7 commits into from

Conversation

vencax
Copy link
Contributor

@vencax vencax commented Jul 21, 2017

Some of webs has divided uploads into separate folders so the media_folder settings needs to be on widget level. This PR solves this without change of original behaviour.
It might be useful for others as well.
Cheers :)

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

@vencax thanks for this! Just a few changes needed :)

@@ -110,6 +110,16 @@ This configuration adds a new setting, `public_folder`. While `media_folder` spe

>If `public_folder` is not set, Netlify CMS will default to the same value as `media_folder`, adding an opening `/` if one is not included.

The same settings can be applied on Image/File widget if you store your file in different folders.
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to keep things minimal in the quick start docs, but this information would be good in docs/widgets.md. Extra sections for the image and file widgets can be added, similar to how the relation widget has a separate section.

@@ -39,20 +41,21 @@ AssetProxy.prototype.toBase64 = function () {
});
};

export function createAssetProxy(value, fileObj, uploaded = false, privateUpload = false) {
export function createAssetProxy(value, fileObj, uploaded = false, field_config = null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

field_config should be field for consistency with other parts of the codebase.

const config = store.getState().config;
const media_folder = field_config && field_config.has('media_folder') ? field_config.get('media_folder') : config.get('media_folder');
const public_folder = field_config && field_config.has('public_folder') ? field_config.get('public_folder') : '/' + media_folder;
Copy link
Contributor

Choose a reason for hiding this comment

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

These can shorten up a bit, e.g.:

const media_folder = field_config && field_config.get('media_folder') || config.get('media_folder');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot see if your version has any sideeffects. I prefer longer but self-explaining version

@vencax
Copy link
Contributor Author

vencax commented Jul 22, 2017

Ok, changed ... cheers :)

const config = store.getState().config;
const media_folder = (field && field.has('media_folder')) ? field.get('media_folder') : config.get('media_folder');
const public_folder = (field && field.has('public_folder')) ? field.get('public_folder') : '/' + media_folder;
Copy link
Contributor

Choose a reason for hiding this comment

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

public_folder now falls back to '/' + media_folder instead of config.get('public_folder')?

@erquhart
Copy link
Contributor

@vencax I pushed some changes to avoid having the asset proxy worry about the concept of a field, and some docs tweaks.

Can you test it against your use case and confirm whether everything is working?

@vencax
Copy link
Contributor Author

vencax commented Jul 24, 2017

Some fixes due to copypaste was needed ... :)

@erquhart
Copy link
Contributor

Ah! Great catches, thank you for that.

@erquhart
Copy link
Contributor

@vencax I'm now thinking we should hold off on this. #350 is a big priority, and allowing multiple media directories becomes much more complex when the Media Library UI is in place.

I'm writing up plans now, but the priority for this goes:

  1. Provide a media library UI that opens when you click an image/file field
  2. Allow multiple asset collections (along with allowing a field to be restricted to a specific asset collection) - this will do what you need
  3. Provide a "front page" UI for asset collections, outside of the pop up UI

Apologies that I didn't already bring this up, didn't consider that there would be a conflict. Please chime in on #350 if you have thoughts on it.

@vencax
Copy link
Contributor Author

vencax commented Jul 26, 2017

ok, i have what I need. It works. You have it in this PR. Do what you consider best for your comunity ...

@erquhart
Copy link
Contributor

erquhart commented Jul 26, 2017

@vencax after #350, the goal of your PR would be addressed by allowing asset collections to be defined in the config, the same way that we have content collections currently. You would also be able to lock a widget to one or more specific asset collections, similar to what you're doing in this PR, except by asset collection name.

Do you think that would work for your use case?

@erquhart erquhart closed this Sep 2, 2017
@ilyabo
Copy link

ilyabo commented Nov 18, 2017

@erquhart I also really need this for making Netlify CMS play with Gatsby nicely. Since an implementation for #350 has been merged, is it now possible to have multiple asset collections and lock a widget to one asset collection as you describe? I tried to find some documentation about this, but couldn't.

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.

3 participants