? Success
Pull Request for # 9639

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
6 Apr 2016

Pull Request for Issue #9639

Summary of Changes

Change from a datalist to a select to be consistent with other dropdowns and to remove confusion mentioned in #9639

Testing Instructions

Make sure that you can add a caption and a class

avatar brianteeman brianteeman - open - 6 Apr 2016
avatar brianteeman brianteeman - change - 6 Apr 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Apr 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 6 Apr 2016
Category UI/UX
avatar brianteeman brianteeman - change - 6 Apr 2016
Rel_Number 0 9639
Relation Type Pull Request for
Easy No Yes
avatar MATsxm MATsxm - test_item - 6 Apr 2016 - Tested successfully
avatar MATsxm
MATsxm - comment - 6 Apr 2016

I have tested this item :white_check_mark: successfully on 29945ed

able to reproduce then #9754 works as expected, able to choose caption class, then change the caption to another, able to save, edit again... everything looks fine here.

Thanks


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

avatar shubhamnba2009 shubhamnba2009 - test_item - 6 Apr 2016 - Tested unsuccessfully
avatar shubhamnba2009
shubhamnba2009 - comment - 6 Apr 2016

I have tested this item :red_circle: unsuccessfully on 29945ed

After applying the patch, as you can see in the image the "caption class" drop down menu still differ from "Image float menu"

<?php echo JText::_('COM_MEDIA_NOT_SET') ?> also not showing "Not set" in the "caption class" drop down menu.
I have tested it in both front-end and back-end article edit menu. Still same issue.

pic


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

avatar MATsxm
MATsxm - comment - 6 Apr 2016

@shubhamnba2009
You applied the patch on 3.5.1?

here's what I have:
screen shot 2016-04-06 at 07 27 31


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

avatar shubhamnba2009
shubhamnba2009 - comment - 6 Apr 2016

@MATsxm I have applied it in 3.5.0 let me test this on 3.5.1. After that i'll submit my test result.


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

avatar shubhamnba2009
shubhamnba2009 - comment - 6 Apr 2016

I have updated my 3.5.0 version to 3.5.1 and after applying the patch still same problem

pic


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

avatar brianteeman
brianteeman - comment - 6 Apr 2016

How are you applying the patch - your screenshot shows the original code

avatar MATsxm
MATsxm - comment - 6 Apr 2016

Caches?
Tested again on Chrome, FF & edge and the patch fix it here :confused:

avatar shubhamnba2009
shubhamnba2009 - comment - 6 Apr 2016

I have updated my Joomla using the update notification available 0n 3.5.0. As you can see in my screeenshot on right side below the version is 3.5.1.
And i had applied the code using Joomla! patch tester!!>

I think there must be some problem while upgrading. Maybe i should download the latest staging then test it!!


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

avatar infograf768
infograf768 - comment - 7 Apr 2016

I can't get the new dropdown and for a good reason:
We have overrides in isis html/media/ that were deleted
https://github.com/joomla/joomla-cms/tree/staging/administrator/templates/isis/html/layouts/joomla
and protostar
https://github.com/joomla/joomla-cms/tree/staging/templates/protostar/html/layouts/joomla

But these files were NOT added to script.php, therefore not deleted when updating Joomla.
Will make an urgent PR.

In the meanwhile, @brianteeman , there is a line in your patch that has extraneous spaces instead of a tab.

+                                           <option value="" selected="selected"><?php echo JText::_('COM_MEDIA_NOT_SET') ?></option>

can you correct?
Once done I can test this OK

avatar joomla-cms-bot
joomla-cms-bot - comment - 7 Apr 2016

This PR has received new commits.

CC: @MATsxm, @shubhamnba2009


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

avatar brianteeman
brianteeman - comment - 7 Apr 2016

Ah that explains why it worked perfectly on a clean install but didnt show on an upgraded site

avatar joomla-cms-bot
joomla-cms-bot - comment - 7 Apr 2016

This PR has received new commits.

CC: @MATsxm, @shubhamnba2009


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

avatar brianteeman
brianteeman - comment - 7 Apr 2016

I have done those two lines but really almost every string needs updating
with it as well. Doesnt fail travis so is it needed?

On 7 April 2016 at 09:38, Joomla! CMS Bot notifications@github.com wrote:

This PR has received new commits.

CC: @MATsxm https://github.com/MATsxm, @shubhamnba2009

https://github.com/shubhamnba2009

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/9754
https://issues.joomla.org/tracker/joomla-cms/9754.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9754 (comment)

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

avatar zero-24
zero-24 - comment - 7 Apr 2016

@brianteeman no. Travis don't check that. I can prepare a seperate PR for that changes at the other places later. As it is out of scope of this PR. Thanks.

avatar brianteeman
brianteeman - comment - 7 Apr 2016

I can add them to this PR if you want and if you say that it is needed

On 7 April 2016 at 09:47, zero-24 notifications@github.com wrote:

@brianteeman https://github.com/brianteeman no. Travis don't check
that. I can prepare a seperate PR for that changes at the other places
later. As it is out of scope of this PR. Thanks.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9754 (comment)

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

avatar infograf768
infograf768 - comment - 7 Apr 2016

PR for the script.php is here>
#9783

avatar mikeveeckmans mikeveeckmans - test_item - 7 Apr 2016 - Tested successfully
avatar mikeveeckmans
mikeveeckmans - comment - 7 Apr 2016

I have tested this item :white_check_mark: successfully on 69f0f8f

TEST OK

Tested on 3.5.1 , PHP 7
screen shot 2016-04-07 at 03 57 12


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

avatar infograf768
infograf768 - comment - 7 Apr 2016

concerning the script PR, I closed it:
My mistake.

These files were NOT present in 3.4.8, 3.5.0 or 3.5.1.
They were remainings of test with the beta/rc

avatar infograf768
infograf768 - comment - 7 Apr 2016

I have tested this item :white_check_mark: successfully on 69f0f8f

Works fine here.


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

avatar infograf768 infograf768 - test_item - 7 Apr 2016 - Tested successfully
avatar brianteeman brianteeman - change - 7 Apr 2016
Status Pending Ready to Commit
avatar brianteeman
brianteeman - comment - 7 Apr 2016

RTC - thanks for testing


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

avatar joomla-cms-bot joomla-cms-bot - change - 7 Apr 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 7 Apr 2016
Milestone Added:
avatar Bakual
Bakual - comment - 9 Apr 2016

The difference between a datalist and a select is that with a datalist you can add an own class there. That may be usefull if your template doesn't support the "text-*" classes or you want to add a custom class there. Originally it was a plain text field if I remember right.

So I don't think it's a good idea to change a text field which allowed any class input to a predefined select.

avatar brianteeman
brianteeman - comment - 9 Apr 2016

How would a user add or remove a class?

On 9 April 2016 at 12:54, Thomas Hunziker notifications@github.com wrote:

The difference between a datalist and a select is that with a datalist you
can add an own class there. That may be usefull if your template doesn't
support the "text-*" classes or you want to add a custom class there.
Originally it was a plain text field if I remember right.

So I don't think it's a good idea to change a text field which allowed any
class input to a predefined select.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9754 (comment)

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

avatar Bakual
Bakual - comment - 9 Apr 2016

Originally it was a plain text field if I remember right.

Turns out I introduced it myself :blush: See #2664 for background.

avatar Bakual
Bakual - comment - 9 Apr 2016

How would a user add or remove a class?

It's a regular textfield, the datalist options only act as suggestions. You can write in anything into that field.

avatar brianteeman
brianteeman - comment - 9 Apr 2016

It's a regular textfield, the datalist options only act as suggestions. You can write in anything into that field.

Clearly several people didnt realise that. Its not a UI convention we use anywhere else so I will stick by my changes in this PR

avatar mbabker
mbabker - comment - 9 Apr 2016

It may not be a common UI convention but IMO this is right as is (unless we duplicate the logic for the module position selector which also allows custom input). I can't say I have the slightest clue where the captions are used (I don't use them, sue me), but there's a part of me that thinks limiting the options to essentially alignment classes might not be a fully wanted behavior.

avatar brianteeman
brianteeman - comment - 9 Apr 2016

in that case I would remove all the options as it certainly isnt clear
that this is not a select but just a suggestion

On 9 April 2016 at 13:25, Michael Babker notifications@github.com wrote:

It may not be a common UI convention but IMO this is right as is (unless
we duplicate the logic for the module position selector which also allows
custom input). I can't say I have the slightest clue where the captions are
used (I don't use them, sue me), but there's a part of me that thinks
limiting the options to essentially alignment classes might not be a fully
wanted behavior.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9754 (comment)

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

avatar Bakual
Bakual - comment - 9 Apr 2016

Clearly several people didnt realise that. Its not a UI convention we use anywhere else so I will stick by my changes in this PR

It's a regular HTML5 element we use there. Nothing special. People will get used to it with time of course.
Either stick with the datalist (select is wrong for sure as it limits the uses of that field) or remove them. But removing will not fix anything imho.
If people are confused about a regular HTML5 feature, then the fix is probably to educate the users :smile:

avatar wilsonge wilsonge - change - 9 Apr 2016
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 9 Apr 2016
Labels Added: ?
avatar wilsonge
wilsonge - comment - 9 Apr 2016

In this case I agree with Thomas - we probably could do more to improve the UX - but it is important that users can optionally enter their own classes in that box (especially for other templates).

avatar wilsonge wilsonge - change - 9 Apr 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-04-09 13:50:21
Closed_By wilsonge
avatar wilsonge wilsonge - close - 9 Apr 2016
avatar joomla-cms-bot joomla-cms-bot - close - 9 Apr 2016
avatar wilsonge wilsonge - close - 9 Apr 2016
avatar joomla-cms-bot joomla-cms-bot - change - 9 Apr 2016
Labels Removed: ?
avatar brianteeman brianteeman - head_ref_deleted - 9 Apr 2016
avatar wilsonge wilsonge - change - 9 Apr 2016
Milestone Removed:

Add a Comment

Login with GitHub to post a comment