Skip to content

Do not hardcode the media manager API url to select an image#39647

Merged
obuisard merged 11 commits intojoomla:4.3-devfrom
Digital-Peak:mm/select-api-url-config
Jan 20, 2023
Merged

Do not hardcode the media manager API url to select an image#39647
obuisard merged 11 commits intojoomla:4.3-devfrom
Digital-Peak:mm/select-api-url-config

Conversation

@laoneo
Copy link
Copy Markdown
Member

@laoneo laoneo commented Jan 16, 2023

Summary of Changes

The media manager is loading their API urls from the JS config store, expect when selecting files in the media form field. This pr extends the media manager the way that the API url is passed to the media field. For backwards compatibility it does a fallback to the old url when it doesn't exist already. This situation can be for older overrides.

@dgrammatiko can you review?

Testing Instructions

Select an intro or full image in the images field.

Actual result BEFORE applying this Pull Request

All works.

Expected result AFTER applying this Pull Request

All works.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev labels Jan 16, 2023
@dgrammatiko
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 2fe8e36


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

@dgrammatiko
Copy link
Copy Markdown
Contributor

Out of curiosity, why did you went with a new entry in the storageOption instead of reusing the existing media-picker key?
Screenshot 2023-01-16 at 17 55 52

@richard67
Copy link
Copy Markdown
Member

@laoneo The JavaScript linter complains: https://ci.joomla.org/joomla/joomla-cms/60937/1/29

@laoneo
Copy link
Copy Markdown
Member Author

laoneo commented Jan 16, 2023

Out of curiosity, why did you went with a new entry in the storageOption instead of reusing the existing media-picker key? Screenshot 2023-01-16 at 17 55 52

Because the old code which adds the entry makes a check if it doesn't exist and then it adds it otherwise not. Using that one makes it unnecessary complex.

@dgrammatiko
Copy link
Copy Markdown
Contributor

Makes sense, thanks

@carlitorweb
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 7cdd1a5


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Jan 16, 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 16, 2023
@carlitorweb
Copy link
Copy Markdown
Member

carlitorweb commented Jan 16, 2023

@dgrammatiko I just see with this PR the TODO comment: // @TODO add this attribute to the field in order to use it for all media types in the layout file. Can be possible this be related with the solution of the issue #35088 ?

@dgrammatiko
Copy link
Copy Markdown
Contributor

Yup, that's the missing part

@carlitorweb
Copy link
Copy Markdown
Member

I wonder if is better insert the types allowed from the options in com_media, instead of hardcode it like this.

laoneo and others added 2 commits January 17, 2023 15:22
…6.js

Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>
…6.js

Co-authored-by: Dimitris Grammatikogiannis <dg@dgrammatiko.dev>
@dgrammatiko
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on aecc96f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

1 similar comment
@viocassel
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on aecc96f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

@dgrammatiko
Copy link
Copy Markdown
Contributor

I wonder if is better insert the types allowed from the options in com_media, instead of hardcode it like this.

@carlitorweb I'm not sure if the MediaHelper returns the list needed here (probably it does because I think I used it in the core media field). Also the implementation should be fairly easy, remove the hardcoded types, add a subform field list for the types (default value to images for b/c). The only thing that might need some thinking is the layout (to keep it 100% b/c), my personal idea would be to keep the default layout for images and add another when there are more options selected (the list field should be multiple, meaning the value could be any combination between images, audios, videos, documents).

@carlitorweb
Copy link
Copy Markdown
Member

@dgrammatiko thank for the hints. I will work with that, and see if I get it done.

@dgrammatiko
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 656f9aa


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39647.

@obuisard obuisard added this to the Joomla! 4.3.0 milestone Jan 20, 2023
@obuisard obuisard merged commit bdf3476 into joomla:4.3-dev Jan 20, 2023
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 20, 2023
@obuisard
Copy link
Copy Markdown
Contributor

Thank you Allon @laoneo !

@laoneo laoneo deleted the mm/select-api-url-config branch January 20, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants