[#26985] - [4.0] Media Info
- Fixed in Code Base
- 6 Nov 2019
- Medium
- Build: 4.0-dev
- # 26985
- Diff
- brianteeman:media_info
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #26690 .
Summary of Changes
The button for image information was being displayed in both the modal and full component view but the actual information had a conditional statement not to be loaded in a modal. As I couldn't see any reason for that condition I removed it so that the information is displayed
Expected result
Clicking on the information icon will display information in both the com_media view and the modal
@wilsonge please check - maybe the condition was there for a reason i cannot see
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_media NPM Change |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26985.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26985.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26985.
IIRC that case (if modal) is used to collapse or do something similar for the pop up window for the media field or the editor media button (yes unfortunately there are 2 different cases for the modal)
@wilsonge With respect to @dgrammatiko 's comment above, should I remove RTC?
@richard67 if you could check the behavior in the media field and reply if anything is broken, I think it would be more helpful
Well the situation is that it is broken BEFORE this PR. Pressing the information button opens the div for the content but doesnt populate it because of the if statement
Confirmed.
@brianteeman i think the idea back then was to maximize the real estate for the modals, thus to skip the info. I think I’ll still be more keen to that idea than having the extra side panel. Also since that thing is bootstrap based I have no clue how it will be rendered in a mobile. Maybe that was the reason for the media manager team opting to hide that panel. Quite honestly it’s been a long time to remember such details
By the way adding the same if statement in the info button should be the solution here
Hmm, both insert image (via CMS content menu) and also insert media field into an article fails with and without this PR.
The image gives an "uncaught exception: undefined" in browser consile, without naming any script, and the media field gives "Error: core.js was not properly initialised" in admin-fields-modal.min.js.
Don't have the time to follow that deeper now, is late here and I have to work tomorrow.
Are there any known issues with that?
Anyway, it seems not to be related to this PR because behavior is same with and without it.
there were two choices. Adding the info or removing the button. I chose to add the info ymmv
Well, I’ve stated the reasons why this was done the other way (mobile users). I guess what the team did back in those days was reflecting the code of the template at that point. Maybe now you can have the side bar in a usable form for mobiles (cannot confirm).
Final word: I’m not against any changes just trying to justify why that code was introduced in the first place. If you have a solid solution that works across all screens then go for it
Whatever they did was incomplete
I've just checked: Mobile modal has already issues without this PR, but with this PR and the info shown it becomes worse.
@brianteeman Do you think we should leave fix of mobile issues in general to another PR, or would you prefer to fix it at least for the info in this PR here? If the latter: Shall I remove RTC until you are ready?
@richard67 the error points that the Joomla object was called in the wrong window (media manager is running in an iframe, so needs to be called as window.parent.Joomla.whatever). Probably my fault here
@dgrammatiko If you can provide a fix I will be happy to test, just ping me ... but not today ... I need sleep soon ;-)
Whatever they did was incomplete
Of course it was incomplete. After that session in Germany there wasn’t any follow up, so, quite frankly the remaining tasks were never been actually worked or completed. This is common pattern for many sub projects for j4. Also I don’t think that there is a media manager group that could finish the remaining tasks...
@richard67 i’m responding through my mobile so that won’t happen today. Maybe tomorrow if I get some time
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-11-06 00:02:31 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
This will do for this one! Thanks!
Thanks
@wilsonge @brianteeman I'm pretty impressed with your point of view here. Did you actually checked that code or it sounded ok-ish, so why not?...
Do this little experiment, add an overflow-x: scroll; to the navigation part of media manager:
scroll till you see that info button and then tap on it. I guess you're fine with the result. It seems that mobile is not important to this project
@dgrammatiko throwing popcorn from the sidelines is easy
throwing popcorn from the sidelines is easy
That's at least unfair mate. I even proposed the right solution: #26985 (comment)
The problem imho is not so much displaying the button for the modal or not but rather in function of the window width when media is displayed, whether it is in the modal of the mobile view.
My guess is that it was intended for the button not to show in the view too?♂ nothing obvious to me anyhow