? Pending

User tests: Successful: Unsuccessful:

avatar Quy
Quy
18 Apr 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">

avatar Quy Quy - open - 18 Apr 2018
avatar Quy Quy - change - 18 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Apr 2018
Category Libraries
avatar Quy Quy - change - 18 Apr 2018
The description was changed
avatar Quy Quy - edited - 18 Apr 2018
avatar brianteeman
brianteeman - comment - 18 Apr 2018

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

avatar dgrammatiko
dgrammatiko - comment - 18 Apr 2018

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

avatar mbabker
mbabker - comment - 18 Apr 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.

avatar dgrammatiko
dgrammatiko - comment - 18 Apr 2018

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

avatar brianteeman
brianteeman - comment - 18 Apr 2018

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

avatar dgrammatiko
dgrammatiko - comment - 18 Apr 2018

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?

avatar ciar4n
ciar4n - comment - 19 Apr 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.

avatar dgrammatiko
dgrammatiko - comment - 19 Apr 2018

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

avatar brianteeman brianteeman - test_item - 6 Jul 2018 - Tested successfully
avatar brianteeman
brianteeman - comment - 6 Jul 2018

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.

avatar mbabker
mbabker - comment - 16 Jul 2018

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

avatar ggppdk ggppdk - test_item - 16 Jul 2018 - Tested successfully
avatar ggppdk
ggppdk - comment - 16 Jul 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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 16 Jul 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 16 Jul 2018

Ready to Commit after two successful tests.

avatar laoneo laoneo - change - 16 Jul 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-07-16 18:59:23
Closed_By laoneo
Labels Added: ?
avatar laoneo laoneo - close - 16 Jul 2018
avatar laoneo laoneo - merge - 16 Jul 2018
avatar laoneo
laoneo - comment - 16 Jul 2018

Good catch, thanks for the fix and the tests.

Add a Comment

Login with GitHub to post a comment