User tests: Successful: Unsuccessful:
Pull Request for Issue #33048 .
Removed class="hasPopover"
, title
and the data-
tags associated with the Popover content from the table header's <a>
tag.
None
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
I have tested this item
This is not correct. You are removing the library so no one can ever use this functionality. You should be removing the implementation of the library in the relevant view
I have tested this item
This is not correct. You are removing the library so no one can ever use this functionality. You should be removing the implementation of the library in the relevant view
I analyzed all the files and views where this library was used and listed it down in this comment #33048 (comment)
I was informed that the popover isn't necessary for any of the views where it is used so I went ahead with this implementation.
And what if an extension was using it?
My opinion is that it wouldn't make much difference to the extension(s) as the popover content was repeating the same title as the table header text and the information text ie. "Select to sort by this column" was a generic statement that can be intuitively understood as the sorting functionality in the table follows the same conventions as the rest of the tables in the site and admin panel.
I am not saying the popover is correct - I am just saying the way you have made this change is not correct
My opinion is that it wouldn't make much difference to the extension
That may or may not be true but you don't just change a library without proper deprecation.
as the sorting functionality in the table follows the same conventions as the rest of the tables in the site and admin panel.
No it doesn't. If you are using a screen reader then there is no way to know what clicking on the column title will do. In the admin there is. Also a site visitor wouldn't know about any convention in the admin ;)
What you should be doing is looking at those other tables in the admin and see how they achieve the same column sorting
@brianteeman I understood your point now and I agree that library helpers cannot be modified without proper deprecation.
I allowed myself to go over the admin panel tables and their implementation of the sorting. I figured out that they make use of searchtools.sort
Helper. This relies on the rendering of a search bar panel for the table's sorting to work as intended.
<?php echo LayoutHelper::render('joomla.searchtools.default', array('view' => $this)); ?>
which outputs
I apologize if I've made a mistake understanding this but I couldn't find a single instance of the sorting table without the search tools panel anywhere in the admin panel. I also believe that the client end of the website has intentionally avoided the use of this search panel which suggests that I could fix given Issue #33048 in 3 ways:
visibility: hidden
on the articles page only so it wouldn't be visible to the users but it'll still be existing for the screen readers. This is a bad approach as it involves overriding of the CSS classes with !importantsearchtools.sort
for the articles table and enclosing the echo
of search panel's LayoutHelper:render
in a div
with display:none
. Again, not the best method but it re-uses the code which avoids the need for a new helper.searchtools.sort
I would like to hear your opinion on this because I consider myself as someone new to Joomla and there is a chance I might've understood something wrong.
You have analyzed and identified the problem correctly but not quite gone far enough. You need to identify what it is in the searchtools layout that provides the functionality we need here. Is it php, js, css etc. When you have done that you will be able to work out the next step that is needed.
Quick Fix methods 1 & 2 are not appropriate. They might be perfectly acceptable things to do on an individual website but I'm sure you will agree are not appropriate for a mass distributed cms
Method 3 is possibly the best option but without the additional analysis its hard to be certain. Maybe its not a helper that is needed but just a single line of js. I don't know myself.
I have not tested this item.
I dwelled further into searchtools
and I believe its dependency on the search/filter panel layout is a reason to avoid that. Rather the best way to resolve the issue is adding a new helper (that is if modifying the existing grid.sort
is not an option). I reckon it will take me some time to do it so I'm closing this PR till then.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-04-15 19:23:21 |
Closed_By | ⇒ | YatharthVyas |
Thank you very much for this, @YatharthVyas - I did not expect that it is so difficult! You have done a good analysis!
I have tested this item✅ successfully on 6726014
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33107.