User tests: Successful: Unsuccessful:
Please go to option=com_cache
page, you will notice that we have to click the checkbox to check every item, there is no use to click item title.
Then we checkout to this PR, now just click item title, the checkbox will be checked.
This is an UX enhancement update.
Some update based on discussion on Tracker
Is there any news about this PR since people has already tested it?
@asika32764 On the tracker item Brian states he doesn't see any changes. So I tested this also and I don't see any changes either. For example when I look at the page administrator/index.php?option=com_cache I notice the table to jump a little but no noticeable difference. In the source I do see the difference, as there is a label tab. What is the use for end-users if they don't see a difference?
When I apply the patch I get a codestyle warning:
:89: space before tab in indent.
warning: 1 line adds whitespace errors.
It will be useful if you can add test instructions, I really don't know what to look for other than may be the label.
@asika32764 I saw the test instructions at the top but I don't see any change in my browser except for looking at the source code. So I am unclear as to what changes I should see. Can you explain that?
OK, the HTML <label>
is an element that direct to an input, and if mouse click this label, the corresponding input will be focused.
Sometimes we write the form like input box
, checkboxes
or radio
, this element are small and hard to select or click.
Label will help us easier to focus on these inputs. For this PR, before label adding, we have to click the checkbox to check it, but after label added, we can just click the title, then the checkbox will be checked.
So we are not able to see any change about the layout, but just use mouse to click table list item, the checkbox will be auto checked.
@asika32764 The test is all good, there is only 1 small problem with the PR. I get this notice:
:89: space before tab in indent.
warning: 1 line adds whitespace errors.
Please fix that and then we can move this forward. Thanks.
Thanks, could you tell me which file?
I don't see any 89 line in changes diff of this PR
Here is the full set I see when I apply and reverse this PR:
D:\wamp\www\joomla-cms>curl https://github.com/joomla/joomla-cms/pull/3154.diff
| git apply
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 10164 100 10164 0 0 16262 0 --:--:-- --:--:-- --:--:-- 20328
:89: space before tab in indent.
warning: 1 line adds whitespace errors.
D:\wamp\www\joomla-cms>curl https://github.com/joomla/joomla-cms/pull/3154.diff
| git apply -R
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 10164 100 10164 0 0 18085 0 --:--:-- --:--:-- --:--:-- 22437
:88: space before tab in indent.
"name, $item->description, 0); ?>"><?php ec
ho $item->name; ?>
warning: 1 line adds whitespace errors.
HTH
Sorry but this information still hard to find the file. I think this warning is not caused by my code, maybe it has already exists from the past. I opened every files I modified and my PHPStorm didn't warning me about Code Style fail.
@asika32764 I found the file causing the problem, administrator/components/com_installer/views/discover/tmpl/default.php that is the culprit. This file has more CS issues, the line you changed may just triggered it. Since you use PhpStorm it might be an idea to go to Edit -> Convert Indents -> To Tabs. Then commit the file again and I can check if the notice is gone.
Thanks for helping, I'm done.
All looks good now, I get no errors when applying the patch. I do see the errors on reverting the patch but they must come from the original file and should be fixed when your PR is committed.
I have set the Feature Request to Accepted, hopefully that is correct ;)
I have set the Feature Request to Accepted, hopefully that is correct ;)
Not really... ☺
Only PLT can do that
So we leave the status at whatever status it is?
Set it to Ready for review
as soon as there are enough tests. It's the equivalent to Ready to commit
.
Merged into 3.4-dev
. Thanks!
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-07-11 06:50:37 |
Great! Commented on tracker.