Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(UX) Add link to edit categories and show parent category #20740

Merged
merged 19 commits into from Jun 29, 2018

Conversation

Projects
None yet
10 participants
@coolcat-creations
Copy link
Contributor

coolcat-creations commented Jun 13, 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

coolcat-creations added some commits Jun 13, 2018

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Jun 13, 2018

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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Jun 13, 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

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 13, 2018

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

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 13, 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?

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 13, 2018

There shouldnt be any queries in a template file.

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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Jun 13, 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

@brianteeman

This comment has been minimized.

Copy link
Contributor

brianteeman commented Jun 13, 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

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Jun 13, 2018

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Jun 13, 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.

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 14, 2018

Thank you both! I will work on it.

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 14, 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.

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 14, 2018

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

grafik

@SharkyKZ

This comment has been minimized.

Copy link
Contributor

SharkyKZ commented Jun 14, 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.

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 14, 2018

Thank you! Done :)

href="<?php echo JRoute::_('index.php?option=com_categories&task=category.edit&id=' . $item->parent_category_id . '&extension=com_content'); ?>"
title="<?php echo JText::_('JACTION_EDIT') . ' ' . JText::_('JCATEGORY'); ?>">

<?php endif ?>

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Add semicolon


<?php endif ?>

<?php echo '' . $item->parent_category_title . ''; ?>

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Remove ''. They are empty strings.

This comment has been minimized.

@coolcat-creations

coolcat-creations Jun 18, 2018

Author Contributor

But there must be a space before the category?

This comment has been minimized.

@coolcat-creations

coolcat-creations Jun 18, 2018

Author Contributor

I see now what you meant ;)

<?php echo JText::_('JCATEGORY') . ': ' . $this->escape($item->category_title); ?>
<?php echo JText::_('JCATEGORY') . ':' ?>

<?php if ($item->parent_category_id != '1') : ?>

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Remove extra space before :

@Quy

This comment has been minimized.

Copy link
Contributor

Quy commented Jun 14, 2018

Remove blank lines between lines 200-241.

@@ -190,7 +197,48 @@
<?php echo JText::sprintf('JGLOBAL_LIST_ALIAS', $this->escape($item->alias)); ?>
</span>
<div class="small">
<?php echo JText::_('JCATEGORY') . ': ' . $this->escape($item->category_title); ?>
<?php echo JText::_('JCATEGORY') . ':' ?>

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Add semicolon

This comment has been minimized.

@Quy

Quy Jun 18, 2018

Contributor

Reminder


<?php if ($canEditParCat || $canEditOwnParCat) : ?>

<a class="hasTooltip"

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Remove extra space


<?php if ($canEditCat || $canEditOwnCat) : ?>

<a class="hasTooltip"

This comment has been minimized.

@Quy

Quy Jun 14, 2018

Contributor

Remove extra space

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 27, 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.

coolcat-creations added some commits Jun 27, 2018

Code Style
Add Feature to Featured Articles Manager
@uglyeoin

This comment has been minimized.

Copy link
Contributor

uglyeoin commented Jun 27, 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.

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 27, 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.

@uglyeoin

This comment has been minimized.

Copy link
Contributor

uglyeoin commented Jun 27, 2018

@infograf768 sorry missed, that, will test again

@uglyeoin

This comment has been minimized.

Copy link
Contributor

uglyeoin commented Jun 27, 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.

endif;
endif;
if ($this->document->direction == "ltr")

This comment has been minimized.

@Quy

Quy Jun 27, 2018

Contributor

Should use isRtl() method to check.

This comment has been minimized.

@infograf768

infograf768 Jun 27, 2018

Member

@Quy
can't we use use both document->direction or
if (JFactory::getLanguage()->isRtl()) ?

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 27, 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.

@infograf768 infograf768 added this to the Joomla 3.9.0 milestone Jun 27, 2018

@roland-d

This comment has been minimized.

Copy link
Contributor

roland-d commented Jun 27, 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
--------------------------------------------------------------------------------
@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 27, 2018

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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.9.0 milestone Jun 27, 2018

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Jun 27, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC label Jun 27, 2018

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 27, 2018

rtc, after the cs is corrected. :)

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 28, 2018

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

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 28, 2018

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

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 28, 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. 👍

@coolcat-creations

This comment has been minimized.

Copy link
Contributor Author

coolcat-creations commented Jun 28, 2018

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

@infograf768 infograf768 added this to the Joomla 3.9.0 milestone Jun 28, 2018

@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jun 28, 2018

Milestoned to 3.9.0.
@mbabker

@mbabker mbabker changed the base branch from staging to 3.9-dev Jun 29, 2018

@mbabker mbabker added PR-3.9-dev and removed PR-staging labels Jun 29, 2018

@mbabker mbabker merged commit a443bbd into joomla:3.9-dev Jun 29, 2018

4 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.