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

[4.0] Remove duplicate class="custom-select" #20196

Merged
merged 1 commit into from Jul 16, 2018
Merged

[4.0] Remove duplicate class="custom-select" #20196

merged 1 commit into from Jul 16, 2018

Conversation

Quy
Copy link
Contributor

@Quy Quy commented Apr 18, 2018

Summary of Changes

Remove duplicate class="custom-select".

Testing Instructions

Go to Menus > Main Menu
View page source
Search for <select id="menutype"
class="custom-select" listed twice.

Expected result

<select id="menutype" name="menutype" class="custom-select" onchange="this.form.submit();">

Actual result

<select id="menutype" name="menutype" class="custom-select" onchange="this.form.submit();" class="custom-select">

@brianteeman
Copy link
Contributor

your pr works - i just wonder if it is correct to remove the one in the library

@dgrammatiko
Copy link
Contributor

i just wonder if it is correct to remove the one in the library

If w don't do that then we're forcing the bootstrap class everywhere which is totally wrong

@mbabker
Copy link
Contributor

mbabker commented Apr 18, 2018

Technically the library helpers shouldn't be framework coupled (even if it is a very generic utility class) unless the helper is a framework oriented one (i.e. JHtmlBootstrap). So it's one of those practical things, if it's in the library there's some Bootstrap leakage there, whereas if it's not then anywhere in the code using this needs to ensure the class is included.

@dgrammatiko
Copy link
Contributor

if it's not then anywhere in the code using this needs to ensure the class is included.

True, and also this gets a bit messy if we consider that these fields get their data from an XML file which is hard to override

@brianteeman
Copy link
Contributor

the reason for me asking was so because i saw other instances of custom-select-* in the libraries and it seems odd to remove one and not the other and smells of an issue elsewhere in the codebase if it is duplicating for this instance but not for others

@dgrammatiko
Copy link
Contributor

Maybe the middle ground will be a joomla-select class (for the core templates will just extend the bootstrap custom-select).
@ciar4n what do you think on this one?

@ciar4n
Copy link
Contributor

ciar4n commented Apr 19, 2018

Maybe the middle ground will be a joomla-select class (for the core templates will just extend the bootstrap custom-select).

I'm not really sure that will help. You go from the field been coupled with bootstrap to been coupled with a block of css. At a basic level we would be asking people to load both bootstrap and this Joomla css (previously 'chosen'). Not that that is a problem but it creates another middle of the road approach. This time between bootstrap and a joomla css framework.

@dgrammatiko
Copy link
Contributor

You go from the field been coupled with bootstrap to been coupled with a block of css.

Well my idea here was about consistency, eg all select elements have this class. What you do with that class is down to the t mplate developer

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on a9e87cb


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

@mbabker
Copy link
Contributor

mbabker commented Jul 16, 2018

x-link comment from duplicate PR: #21147 (comment)

@ggppdk
Copy link
Contributor

ggppdk commented Jul 16, 2018

I have tested this item ✅ successfully on a9e87cb

While testing our extension with J4, i came across this one,
specifying a custom class for select.groupedlist is problematic as you have duplicate class attribute added

The hard coded class attribute is not added for select.list, but only for select.groupedlist, such hard coding does not exist in J3
and in the end if it is really needed, then only add it only as default if one was not provided by the method caller


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

@ghost
Copy link

ghost commented Jul 16, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 16, 2018
@laoneo laoneo added this to the Joomla 4.0 milestone Jul 16, 2018
@laoneo laoneo merged commit db28079 into joomla:4.0-dev Jul 16, 2018
@joomla-cms-bot joomla-cms-bot added PR-4.0-dev and removed RTC This Pull Request is Ready To Commit labels Jul 16, 2018
@laoneo
Copy link
Member

laoneo commented Jul 16, 2018

Good catch, thanks for the fix and the tests.

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.

None yet

8 participants