? Success

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
24 Aug 2016

Summary of Changes

Upgrade Chosen to the latest release (1.6.1)
Revert the change of class and function names to ensure full backwards compatability
Add the two documented hacks we made in the past

Testing Instructions

chosen is used throughout the cms for styling dropdown menus
Pay particular attention to the search tools and the ability to create categories, module positions and tags "on the fly" - these are both ares our hack was used for,

Documentation Changes Required

None

avatar joomla-cms-bot joomla-cms-bot - change - 24 Aug 2016
Category JavaScript
avatar brianteeman brianteeman - open - 24 Aug 2016
avatar brianteeman brianteeman - change - 24 Aug 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Aug 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 24 Aug 2016
The description was changed
avatar brianteeman brianteeman - edited - 24 Aug 2016
avatar brianteeman
brianteeman - comment - 24 Aug 2016

It might be a good idea if someone double checks the PRs that hacked chosen in the first place to make sure I got everything as I only ported across changes that were documented in the source code.

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Aug 2016

hum ... why was chosen "hacked" in the first place? .... (don't awnser that is just a rethorical question) ?

My question is: instead of "rehacking" in a new version can't joomla somehow extend default chosen.js in a new js file to make the changes joomla needs?

For what i understand the changes are this: 0ef056d

@dgt41 @Fedik any toughts?

avatar dgt41
dgt41 - comment - 24 Aug 2016

I was about to make a PR with https://github.com/okcoker/taggle.js for tags and drop chosen from there (that's for J4). That script is vanilla, tiny and it works wonderfully.
Now for the other places that chosen is required (why do we use chosen so extensively?) I have no clue if the applied joomla patch would be required but reading the code we should be able to extend by replacing the relative prototype.

avatar mbabker
mbabker - comment - 24 Aug 2016

why do we use chosen so extensively?

Style decision made in 2012.

avatar dgt41
dgt41 - comment - 24 Aug 2016

@andrepereiradasilva now that I spend some time on it (reading the code) I realise that we are about to fork chosen since we keep the old class names, so I guess this approach of extending is not valid anyway ?

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Aug 2016

ok then

avatar brianteeman
brianteeman - comment - 24 Aug 2016

Hacking it was a mistake. But then we were screwed when they changed all
the class names anyway.

avatar brianteeman
brianteeman - comment - 24 Aug 2016

Discussing changing from chosen is not for this PR and cant be done until 4.0

Tests for this PR would be more productive at this time

avatar wilsonge
wilsonge - comment - 24 Aug 2016

This PR is fine in principle for the 3.x branch and the PLT approved this change before Brian made the PR. As we're changing pretty much every function because of the classes and events changes in chosen 1. So there's no point in creating a wrapper.

I think we all know that chosen will be dropped in Joomla 4 - or at least substantially reduced in scale. But 3.x isn't going to go away any time soon and this PR is a good improvement until then.

avatar andrepereiradasilva
andrepereiradasilva - comment - 24 Aug 2016

Ok will test when have time

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

hum ... why was chosen "hacked" in the first place?

Good question. I'm going to try and prevent this from happening in J4.x and throw abuse at anyone that even hints at hacking any JS/CSS files


@brianteeman - Can I test this PR now?

avatar brianteeman
brianteeman - comment - 25 Aug 2016

Yes it is ready for testing

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

Getting this in the console after waiting about 5 seconds on the Control Panel:

screeny

avatar brianteeman
brianteeman - comment - 25 Aug 2016

I dont get that in chrome but I can confirm it in firefox - however can you
confirm if this is present before applying this patch. It looks like it is
not related as the debugger in FF doesnt highlight the chosen js and I see
it on none patched sites. (I think it is related to update check)

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

Ah yes sorry, my bad. This error appears when you start clicking anywhere on the page, so it's not related.

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

Ok everything seems to be working fine.

Just 1 small CSS tweak that may need doing. You get a 1px bit of whitespace above the green background on the dropdown:

screeny

avatar brianteeman
brianteeman - comment - 25 Aug 2016

Again - is that css "error" present before this PR

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

No, only after the patch

avatar brianteeman
brianteeman - comment - 25 Aug 2016

I can confirm that @c-lodder but those css fixes are above my pay grade and skill level


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

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

@brianteeman - Remove box-shadow: 0 1px 0 #fff inset;

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2016
Category JavaScript Front End Components JavaScript
avatar brianteeman
brianteeman - comment - 25 Aug 2016

Made that change @C-Lodder

avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

Nice.

What are the PHP changes you've added doing?

avatar brianteeman
brianteeman - comment - 25 Aug 2016

Shouldnt be there - oops

On 25 August 2016 at 14:18, Lodder notifications@github.com wrote:

Nice.

What are the PHP changes you've added doing?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11779 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VyILpu_yOnaDxFaSKbvHqFR2Frgks5qjZYygaJpZM4JsMWq
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2016
Category JavaScript Front End Components JavaScript
avatar brianteeman
brianteeman - comment - 25 Aug 2016

@C-Lodder fixed that mistake

avatar C-Lodder C-Lodder - test_item - 25 Aug 2016 - Tested successfully
avatar C-Lodder
C-Lodder - comment - 25 Aug 2016

I have tested this item successfully on 48051c7


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

avatar MATsxm MATsxm - test_item - 25 Aug 2016 - Tested successfully
avatar MATsxm
MATsxm - comment - 25 Aug 2016

I have tested this item successfully on 48051c7


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

avatar zero-24 zero-24 - change - 25 Aug 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 25 Aug 2016

RTC. Thanks


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

avatar joomla-cms-bot joomla-cms-bot - change - 25 Aug 2016
Labels Added: ?
avatar bertmert
bertmert - comment - 26 Aug 2016

@brianteeman
I personally don't care but found a small issue. If you think that should be fixed:

Firefox 48

  • Go to Articles Manager.
  • Search tools: Select category "Components".
  • Have a look on the "p" and padding-bottom

Before patch

26-08-_2016_10-16-43

After patch

26-08-_2016_10-14-23

avatar ciar4n
ciar4n - comment - 29 Aug 2016

Appears to be an issue in the tags field if you start to type in a tag name that already exists...

chosen

The typed tag gets cropped to 25px wide.

<input class="" type="text" style="width: 25px;" autocomplete="off" value="ciaran">
avatar ciar4n
ciar4n - comment - 29 Aug 2016

Issue present without the patch so created a new issue.. #11835

avatar wilsonge
wilsonge - comment - 31 Aug 2016

Merged into 3.7 with 08f116c

avatar wilsonge wilsonge - change - 31 Aug 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-08-31 23:05:00
Closed_By wilsonge
avatar wilsonge wilsonge - close - 31 Aug 2016
avatar joomla-cms-bot joomla-cms-bot - close - 31 Aug 2016
avatar joomla-cms-bot joomla-cms-bot - change - 31 Aug 2016
Labels Removed: ?
avatar brianteeman
brianteeman - comment - 31 Aug 2016

Woot. Thank you

On 1 Sep 2016 12:05 a.m., "George Wilson" notifications@github.com wrote:

Merged into 3.7 with 08f116c
08f116c


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11779 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VZKw_nek_lxTnZUENyH5FV6bloSks5qlgidgaJpZM4JsMWq
.

avatar demis-palma
demis-palma - comment - 6 Jun 2017

I'm sorry, I haven't seen this PR up to now.
It actually causes a regression. See #16552

Add a Comment

Login with GitHub to post a comment