? Pending

User tests: Successful: Unsuccessful:

avatar YatharthVyas
YatharthVyas
11 Apr 2021

Pull Request for Issue #33048 .

Summary of Changes

Removed class="hasPopover", title and the data- tags associated with the Popover content from the table header's <a> tag.

Testing Instructions

  1. Add Category List to frontend (Joomla Admin Panel -> Menus -> Pick a Menu -> Add Menu Item -> Category List)
  2. Visit the frontend page and hover over the table's header.

Actual result BEFORE applying this Pull Request

image

Expected result AFTER applying this Pull Request

output)

Documentation Changes Required

None

avatar YatharthVyas YatharthVyas - open - 11 Apr 2021
avatar YatharthVyas YatharthVyas - change - 11 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 11 Apr 2021
Category Libraries
avatar ChristineWk ChristineWk - test_item - 11 Apr 2021 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 11 Apr 2021

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.

avatar YatharthVyas YatharthVyas - change - 11 Apr 2021
Labels Added: ?
avatar ChristineWk ChristineWk - test_item - 11 Apr 2021 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 11 Apr 2021

I have tested this item successfully on df491d3


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

avatar brianteeman
brianteeman - comment - 12 Apr 2021

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

avatar brianteeman brianteeman - test_item - 12 Apr 2021 - Tested unsuccessfully
avatar brianteeman
brianteeman - comment - 12 Apr 2021

I have tested this item ? unsuccessfully on df491d3


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

avatar YatharthVyas
YatharthVyas - comment - 12 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 12 Apr 2021

And what if an extension was using it?

avatar YatharthVyas
YatharthVyas - comment - 12 Apr 2021

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.

avatar brianteeman
brianteeman - comment - 12 Apr 2021

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

avatar YatharthVyas
YatharthVyas - comment - 13 Apr 2021

@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

image

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:

  1. Quick Fix Method: Using CSS to hide the popover text using 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 !important
  2. Quick Fix Method 2: Using searchtools.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.
  3. Best Method: Making a new helper function that performs all the tasks of a grid.sort helper but doesn't display the popover text. Instead, it informs the screen readers differently by using a hidden span similar to how it is done in 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.

avatar brianteeman
brianteeman - comment - 13 Apr 2021

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.

avatar ChristineWk ChristineWk - test_item - 13 Apr 2021 - Not tested
avatar ChristineWk
ChristineWk - comment - 13 Apr 2021

I have not tested this item.


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

avatar YatharthVyas
YatharthVyas - comment - 15 Apr 2021

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.

avatar YatharthVyas YatharthVyas - close - 15 Apr 2021
avatar YatharthVyas YatharthVyas - change - 15 Apr 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-04-15 19:23:21
Closed_By YatharthVyas
avatar chmst
chmst - comment - 15 Apr 2021

Thank you very much for this, @YatharthVyas - I did not expect that it is so difficult! You have done a good analysis!

Add a Comment

Login with GitHub to post a comment