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
Merged

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

merged 19 commits into from Jun 29, 2018

Conversation

coolcat-creations
Copy link
Contributor

@coolcat-creations 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

@brianteeman
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

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
Copy link
Contributor Author

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

@coolcat-creations
Copy link
Contributor Author

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
Copy link
Contributor Author

There shouldnt be any queries in a template file.

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

@brianteeman
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Query should be in articles model. See frontend articles model for example:
https://github.com/coolcat-creations/joomla-cms/blob/2334e0d888129ba2b4f42ec056b9cfe2086098ed/components/com_content/models/articles.php#L242-L244

@SharkyKZ
Copy link
Contributor

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
Copy link
Contributor Author

Thank you both! I will work on it.

@coolcat-creations
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
Copy link
Contributor Author

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

grafik

@SharkyKZ
Copy link
Contributor

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
Copy link
Contributor Author

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 ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add semicolon


<?php endif ?>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove ''. They are empty strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there must be a space before the category?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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') : ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra space before :

@Quy
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') . ':' ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add semicolon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder


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

<a class="hasTooltip"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra space


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

<a class="hasTooltip"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra space

@Quy
Copy link
Contributor

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

@coolcat-creations
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.

Add Feature to Featured Articles Manager
@uglyeoin
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

@infograf768 sorry missed, that, will test again

@uglyeoin
Copy link
Contributor

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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use isRtl() method to check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@infograf768
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

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
@ghost
Copy link

ghost commented Jun 27, 2018

Ready to Commit after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 27, 2018
@infograf768
Copy link
Member

infograf768 commented Jun 27, 2018

rtc, after the cs is corrected. :)

@infograf768
Copy link
Member

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

@coolcat-creations
Copy link
Contributor Author

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

@infograf768
Copy link
Member

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
Copy link
Contributor Author

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
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 June 29, 2018 01:50
@mbabker mbabker merged commit a443bbd into joomla:3.9-dev Jun 29, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants