? ? Pending

User tests: Successful: Unsuccessful:

avatar coolcat-creations
coolcat-creations
13 Jun 2018

Summary of Changes

This PR adds the Information to the parent category of the article before the category and offers a direct link to edit it.

Testing Instructions

Small change, big UX improvement
-> access the categories easier and faster
-> Some pages have similar named subcategories so its better to know the parent too

Please test coding style and everything, I did it how I thought it might work... thanks a lot!

grafik

avatar coolcat-creations coolcat-creations - open - 13 Jun 2018
avatar coolcat-creations coolcat-creations - change - 13 Jun 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 13 Jun 2018
Category Administration com_content
avatar coolcat-creations coolcat-creations - change - 13 Jun 2018
Labels Added: ?
avatar coolcat-creations coolcat-creations - change - 13 Jun 2018
The description was changed
avatar coolcat-creations coolcat-creations - edited - 13 Jun 2018
avatar brianteeman
brianteeman - comment - 13 Jun 2018

Do we need to do something like just displaying the last x elements in the category tree if it is very long

avatar brianteeman
brianteeman - comment - 13 Jun 2018

There shouldnt be any queries in a template file.
For the template file please look your code and make sure that it is in a nested structure
You should probably update the featured articles view as well

Finally you are making all the categories a link - shouldnt the link be disabled if the user does not have permission to edit the category

avatar coolcat-creations
coolcat-creations - comment - 13 Jun 2018

it's just the parent of the last one... because i think it could indeed be very long

avatar coolcat-creations
coolcat-creations - comment - 13 Jun 2018

Finally you are making all the categories a link - shouldnt the link be disabled if the user does not have permission to edit the category

I thought i am doing that check - is it done wrong?

avatar coolcat-creations
coolcat-creations - comment - 13 Jun 2018

There shouldnt be any queries in a template file.

Can someone give me advise where and how to put the query in ?

avatar brianteeman
brianteeman - comment - 13 Jun 2018

I only had a chance to read the code and with the current formatting it's very hard to spot everything. Hence my earlier mistaken comments.

If you could do a quick update on just the code formatting it will make it easier

avatar brianteeman
brianteeman - comment - 13 Jun 2018

Do you think it would be helpful to have a visual indicator if the two categories shown are NOT at the root.

With PR

chrome_2018-06-13_20-14-03

Suggestion

chrome_2018-06-13_20-14-51

avatar SharkyKZ
SharkyKZ - comment - 13 Jun 2018
avatar SharkyKZ
SharkyKZ - comment - 13 Jun 2018

For permission check to work you need to get permissions for each category. Currently, $canEdit and $canEditOwn variables hold permissions of article.

ROOT check should be performed against category ID as opposed to category title because it's possible to create a category titled ROOT.

avatar coolcat-creations
coolcat-creations - comment - 14 Jun 2018

Thank you both! I will work on it.

avatar coolcat-creations
coolcat-creations - comment - 14 Jun 2018

I put the query into the model, next steps will be the other improvements (acl and levelindicator) thanks again for your review!

Edit: levelidicator is done, Now my articletitle is messed up ;) will fix it.

avatar coolcat-creations
coolcat-creations - comment - 14 Jun 2018

I guess I am all set now, can you please review my code once more? Thank you!!!

grafik

avatar SharkyKZ
SharkyKZ - comment - 14 Jun 2018

The joins for category authors is unnecessary. The 4 category related blocks can be replaced with these 2:

// Join over the categories.
$query->select('c.title AS category_title, c.created_user_id as category_uid')
	->join('LEFT', '#__categories AS c ON c.id = a.catid');

// Join over the parent categories.
$query->select('parent.title AS parent_title, parent.id AS parent_id, parent.created_user_id AS parent_uid')
	->join('LEFT', '#__categories AS parent ON parent.id = c.parent_id');

This also removes some unnecessary selects, e.g. category ID is already selected as catid.

avatar coolcat-creations
coolcat-creations - comment - 14 Jun 2018

Thank you! Done :)

avatar Quy
Quy - comment - 14 Jun 2018

Remove blank lines between lines 200-241.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 16 Jun 2018

@coolcat-creations is this PR ready for Test??

avatar coolcat-creations
coolcat-creations - comment - 18 Jun 2018

@franz-wohlkoenig yes ready to test, @Quy thanks for reviewing the Code Style!

avatar uglyeoin uglyeoin - test_item - 18 Jun 2018 - Tested successfully
avatar uglyeoin
uglyeoin - comment - 18 Jun 2018

I have tested this item โœ… successfully on 8d5023c

Test successful works with categories/child categories, shows the correct stuff, has Brians suggestion implemented (nice suggestion Brian). Successful test.


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

avatar infograf768
infograf768 - comment - 18 Jun 2018

Has this been tested in RTL?

avatar coolcat-creations
coolcat-creations - comment - 18 Jun 2018

@infograf768 I am afraid - no - and seems thats a good point to keep care of, I'll make some changes! Thanks!!!

avatar infograf768
infograf768 - comment - 18 Jun 2018

This is what I get with Persian
screen shot 2018-06-18 at 16 11 38

Not only the chevron is wrong, but also the order of parent category and category.
If you want to keep the chevron after modification, I suggest you rather use an htmlentity btw.

avatar coolcat-creations
coolcat-creations - comment - 18 Jun 2018

Perfect, thank you, I was not aware of rtl but will keep that always in mind from now!

avatar coolcat-creations
coolcat-creations - comment - 22 Jun 2018

Just a small update: I have Issues installing a new language in staging, I will have to reinstall everything and then implement the rtl stuff. I shall find some time next week!

avatar brianteeman
brianteeman - comment - 22 Jun 2018

You do not need to install a new language to test something in RTL
\administrator\language\en-GB\en-GB.xml

change
<rtl>0</rtl>
to
<rtl>1</rtl>

avatar infograf768
infograf768 - comment - 22 Jun 2018

You do not need to install a new language to test something in RTL

Not in this case as the rtl word category with : in RTL displays differently. I tested.
Test can be done with English, than keeping the page in its window and opening a new window displaying admin installed languages and switching to Arabic or Persian, then reloading the other window, etc.

avatar coolcat-creations
coolcat-creations - comment - 25 Jun 2018

I changed everything to work in rtl - can you check and test? Thanks @uglyeoin @infograf768

avatar infograf768
infograf768 - comment - 26 Jun 2018

Nice. It works.
Some suggestions : I would use hmtlentities for ยซ ยป. A matter of choice I guess.

Code style in the default.php
blank lines before the if (
move { and } to the next line.

I let @Quy propose solutions for that. ๐Ÿ˜ƒ

More important: please do the same for the Featured articles view.

avatar Quy
Quy - comment - 27 Jun 2018

While it is convenient, how often are categories updated? Usually, they are set up and forget. This PR will add extra markup to the page that will probably not be used majority of the time.

avatar coolcat-creations
coolcat-creations - comment - 27 Jun 2018

@Quy Explaining Joomla to others and giving training brought me to that point. While you can jump to the articles from the Categories you can't access the Category from the Article. The intention is to enable the User to keep more care of the categories and als not to setup and forget. The extra Level brings sureness that they edit the right thing. Sometimes it's not clear just from one level. I think the markup is not blown very much.

avatar uglyeoin uglyeoin - test_item - 27 Jun 2018 - Tested unsuccessfully
avatar uglyeoin
uglyeoin - comment - 27 Jun 2018

I have tested this item ๐Ÿ”ด unsuccessfully on ef35014

Sorry Elisa, this one failed. See this image for details:

https://www.dropbox.com/s/1nusc6o3iqp0ui9/elisa-categories-pull-request.png?dl=0

Looks like a small amendment. Let me know and I will test again.


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

avatar infograf768
infograf768 - comment - 27 Jun 2018

@uglyeoin
As i explained above, you cannot test this one by just setting en-GB to Rtl.
please install persian or Arabic and test again.

avatar uglyeoin
uglyeoin - comment - 27 Jun 2018

@infograf768 sorry missed, that, will test again

avatar uglyeoin uglyeoin - test_item - 27 Jun 2018 - Tested successfully
avatar uglyeoin
uglyeoin - comment - 27 Jun 2018

I have tested this item โœ… successfully on ef35014

Successfully tested in Persian


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

avatar infograf768 infograf768 - test_item - 27 Jun 2018 - Tested successfully
avatar infograf768
infograf768 - comment - 27 Jun 2018

I have tested this item โœ… successfully on ef35014


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

avatar roland-d
roland-d - comment - 27 Jun 2018

@coolcat-creations Codestyle failures:

FILE: ...mla/joomla-cms/administrator/components/com_content/models/featured.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 111 | WARNING | Line exceeds 150 characters; contains 176 characters
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: ...mla/joomla-cms/administrator/components/com_content/models/articles.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 213 | WARNING | Line exceeds 150 characters; contains 176 characters
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------
avatar infograf768
infograf768 - comment - 27 Jun 2018

Well thought. Indeed drone is there to remind us.. :)

avatar franz-wohlkoenig franz-wohlkoenig - change - 27 Jun 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 Jun 2018

Ready to Commit after two successful tests.

avatar infograf768
infograf768 - comment - 27 Jun 2018

rtc, after the cs is corrected. :)

avatar infograf768
infograf768 - comment - 28 Jun 2018

@coolcat-creations
Can you correct the cs so that the PR is complete?

avatar coolcat-creations coolcat-creations - change - 28 Jun 2018
Labels Added: ?
avatar coolcat-creations
coolcat-creations - comment - 28 Jun 2018

I corrected the lines, do I have to use the other rtl method too?

avatar infograf768
infograf768 - comment - 28 Jun 2018

I corrected the lines, do I have to use the other rtl method too?

We use both methods in core. the most commonly used one is indeed JFactory::getLanguage()->isRtl()
You decide. ๐Ÿ‘

avatar coolcat-creations
coolcat-creations - comment - 28 Jun 2018

Ok I used the more common method :)
Thank you all for testing and supporting, I learned a lot with this PR.

avatar infograf768
infograf768 - comment - 28 Jun 2018

Milestoned to 3.9.0.
@mbabker

avatar mbabker mbabker - close - 29 Jun 2018
avatar mbabker mbabker - merge - 29 Jun 2018
avatar mbabker mbabker - change - 29 Jun 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-29 01:50:32
Closed_By mbabker
Labels Added: ?
Removed: ?

Add a Comment

Login with GitHub to post a comment