Skip to content

[4.2] Remove ID attribute from mod_custom#37976

Closed
Fedik wants to merge 10 commits intojoomla:4.2-devfrom
Fedik:mod-custom-id
Closed

[4.2] Remove ID attribute from mod_custom#37976
Fedik wants to merge 10 commits intojoomla:4.2-devfrom
Fedik:mod-custom-id

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Jun 4, 2022

Summary of Changes

It redo of #32980, but without use of ID attribute

Testing Instructions

Select background Image for mod_custom,
And check module on the site, also the module HTML markup

Actual result BEFORE applying this Pull Request

Background visible, module have an ID attribute

Expected result AFTER applying this Pull Request

Background visible, module do not have an ID attribute

Documentation Changes Required

none

@Fedik Fedik requested a review from chmst as a code owner June 4, 2022 06:57
@Fedik Fedik changed the base branch from 4.1-dev to 4.2-dev June 4, 2022 07:32
@Fedik Fedik changed the title [4.1] Remove ID attribute from mod_custom [4.2] Remove ID attribute from mod_custom Jun 4, 2022
@Quy Quy removed the PR-4.1-dev label Jun 5, 2022
@brianteeman
Copy link
Contributor

The changes to administrator/components/com_installer/src/Controller/InstallController.php look unrelated to me

Fedik added 2 commits June 12, 2022 10:14
 Conflicts:
	templates/cassiopeia/html/mod_custom/banner.php
@Fedik
Copy link
Member Author

Fedik commented Jun 12, 2022

That because it was for 4.1 first.
Should be good now.

@simbus82
Copy link
Contributor

Why the ID needs to be removed?

@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@chmst
Copy link
Contributor

chmst commented Jun 28, 2022

@simbus82 This is why: #37873 (comment) - multiple IDs are possible.

@Fedik
Do we need an extra classname? Other modules don't.
The variablename $modid sounds like and ID but makes a classname.

@Fedik
Copy link
Member Author

Fedik commented Jun 28, 2022

Do we need an extra classname? Other modules don't.

The code changes use of ID to class-[id]

@simbus82
Copy link
Contributor

@simbus82 This is why: #37873 (comment) - multiple IDs are possible.

Ok, it's clear that we can't have multiple ID in a single DOM (but only for validation): in 2022 is only a problem if we try to pointing them wrongly with CSS and JS.
Removing an ID (in the core since one year) can break lot's of templates that rely on this method to apply some css styles.
I hope users will be informed about this B/C.

@brianteeman
Copy link
Contributor

As pointed out before by others removing the id will be an isssue for any site that is using the id for css and/or js (I am) and while it can be argued that this is not covered by the b/c promise is it really necessary?

The only way that you can have one module (with the same id) published more than once is to additionally load the module in some content with the loadmodule plugin. As that is a deliberate action by the web developer to publish the module twice on the same page when it makes no sense to do so I would expect the occurance of this to be infinitely less than the use of the id or js/css

@Fedik
Copy link
Member Author

Fedik commented Jul 2, 2022

I just sugested a solution to avoid duplicated ids (even if it rare possible) :)
It can be accepted or closed, I am fine with both.

@HLeithner
Copy link
Member

Mod_custom is the module used as reference for new module that should not suggest to use wrong syntax.

@Fedik
Copy link
Member Author

Fedik commented Jul 2, 2022

For keep b/c I can revert ID (for people who use js), but the styling (background image) will be done with class. And will remove ID in j5 (if no one forget).
Deal? :)

@Mrsuperstar304
Copy link

Hey everyone, I am new here. Please tell me how to start here and what are the skills required to contribute here.


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

@chmst
Copy link
Contributor

chmst commented Jul 7, 2022

@Mrsuperstar304 thank you for your interest in contributing to Joomla. Maybe you start with this video https://www.youtube.com/watch?v=30JYBOeNVTA. Or search for "how to contribute " in joomla documentation.

@simbus82
Copy link
Contributor

simbus82 commented Jul 7, 2022

Mod_custom is the module used as reference for new module that should not suggest to use wrong syntax.

And it's a wrong syntax to not have an unique ID for every instance of rendered mod_custom in the DOM 😄

@Fedik
Copy link
Member Author

Fedik commented Jul 10, 2022

I have restored ID, and added a comment that it should be removed in next Major version

@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@Fedik Fedik closed this Mar 22, 2023
@Fedik Fedik deleted the mod-custom-id branch March 22, 2023 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Small A PR which only has a small change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comments