Skip to content

Comments

[4.1] Support class attribute in media custom field#36593

Closed
laoneo wants to merge 8 commits intojoomla:4.1-devfrom
Digital-Peak:j4/fields/media-class
Closed

[4.1] Support class attribute in media custom field#36593
laoneo wants to merge 8 commits intojoomla:4.1-devfrom
Digital-Peak:j4/fields/media-class

Conversation

@laoneo
Copy link
Member

@laoneo laoneo commented Jan 7, 2022

Pull Request for pr #27712.

Summary of Changes

The accessible media form field doesn't support the class property. This pr fixes it.

Testing Instructions

  • Create a media custom field
  • In the options of the field set the "Field Class" attribute to testclass
  • Save the field
  • Edit an article
  • Inspect the joomla-field-media element of the custom field in the dev tools of the browser

Actual result BEFORE applying this Pull Request

No testclass is added to the element.

Expected result AFTER applying this Pull Request

testclass is added to the element.

@laoneo
Copy link
Member Author

laoneo commented Jan 7, 2022

@astridx can you have a look here if all is correct?

@alikon
Copy link
Contributor

alikon commented Jan 9, 2022

I have tested this item ✅ successfully on e4f4e2d


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

@BPBlueprint
Copy link

I have tested this item ✅ successfully on 87ab096

Tested successfully.


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

@alikon
Copy link
Contributor

alikon commented Jan 10, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 10, 2022
@wilsonge
Copy link
Contributor

This looks kinda weird because the class is already being set on the input element within the custom form field. Does it really make sense to have it in both places?

@laoneo
Copy link
Member Author

laoneo commented Jan 24, 2022

Did you actually test it? The class is not set, as it is not passed into the subform group. It is not rendered at all in any of the elements.

@Quy Quy changed the base branch from 4.0-dev to 4.1-dev January 27, 2022 20:10
@Quy Quy added PR-4.1-dev and removed PR-4.0-dev labels Jan 28, 2022
@wilsonge
Copy link
Contributor

Yes I understand that the FormField layout is clearly correct. I'm asking specifically about the JLayout change as the class is also used here https://github.com/joomla/joomla-cms/pull/36593/files#diff-ffc48fe23d772f6461955e77801148ec01945822ae6574126b6cece7f7bccd26R54 and you're now adding it again on the custom element.

@laoneo
Copy link
Member Author

laoneo commented Jan 29, 2022

The class is only added to the custom element and nowhere else. The class is delegated to the element from the Formfield but never used there as you can see here https://github.com/joomla/joomla-cms/blob/4.1-dev/layouts/joomla/form/field/media/accessiblemedia.php. So where should then the class being used in your opinion?

@richard67 richard67 changed the title [4.0] Support class attribute in media custom field [4.1] Support class attribute in media custom field Jan 31, 2022
@laoneo
Copy link
Member Author

laoneo commented Feb 23, 2022

Made another test and indeed you are right, the class is used on the input. So it would be wrong to render it two times.

@laoneo laoneo closed this Feb 23, 2022
@laoneo laoneo deleted the j4/fields/media-class branch February 23, 2022 05:00
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 23, 2022
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.

6 participants