? PR-4.4-dev Pending

User tests: Successful: Unsuccessful:

avatar nasirkhan
nasirkhan
24 Aug 2023

Summary of Changes

For the List All Tags menu type, there are options to set a number of columns. But at present, this does not reflect on the view. This PR will fix this issue and column changes will be reflected on the view.

I consider this as a bug fix as the current menu setting is not reflected in the view. This might be a breaking change, for some websites as the HTML structure will be changed.

This Pull Request will fix a partial of the Issue #39177 .

Testing Instructions

  1. Create a new menu item as List All Tags
  2. Change the Number of Columns from the Options tab
  3. Follow the changes in column in the view

Actual result BEFORE applying this Pull Request

Showing all the tags in a single column without respecting the Number of Columns in the Options tab.

Expected result AFTER applying this Pull Request

Number of Columns in the Options tab will be reflected in the view.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 24 Aug 2023
Category com_tags Front End
avatar nasirkhan nasirkhan - open - 24 Aug 2023
avatar nasirkhan nasirkhan - change - 24 Aug 2023
Status New Pending
avatar laoneo
laoneo - comment - 26 Aug 2023

Thanks for the pr, can you not make it in a backwards compatible way?

avatar nasirkhan
nasirkhan - comment - 26 Aug 2023

Thanks for the pr, can you not make it in a backwards compatible way?

Thanks for your comment. I can't understand what you are saying here. are you asking me to make it backward compatible?

At this moment the number of columns compared with Bootstrap 12 column grid. So for me, using the row-column combination is the right way forward. We may add more options for mobile and tab screen column count and append the classes accordingly.

avatar hans2103
hans2103 - comment - 11 Sep 2023

@nasirkhan
Using an unordered list (<ul>) makes it possible for screen readers to calculate the amount of list items (<li>). Convert it to a <div> makes it an empty box not knowing how many items are in the list. I prefer to use the unordered list and would recommend not to merge this PR as it is right now.

avatar laoneo
laoneo - comment - 11 Sep 2023

So I'm closing this pr. You can always use an override with your changes. I guess we should tackle the issue in a different way.

avatar laoneo laoneo - close - 11 Sep 2023
avatar laoneo laoneo - change - 11 Sep 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-09-11 13:43:25
Closed_By laoneo
Labels Added: ? PR-4.4-dev
avatar nasirkhan
nasirkhan - comment - 11 Sep 2023

thank you for your thoughtful comments.

I think we @laoneo and @hans2103 none of you understood the issue this pull request solves. There is a bug, which is The Number of Columns from the Options tab does not have any effect in the frontend view. This pull request solves this, without changing anything in the admin backend. It continues the logic of 12-column grid and follows the Bootstrap recommended approach to creating rows and columns.

@hans2103 mentioned that the current bug is helpful for better accessibility and his personal preferences in using ul. If accessibility is the concern that can be solved by using a dedicated aria tags like aria-colcount. Why don't you want to use such HTML tags which are created to solve accessibility issues?

@laoneo Can you please open the pull request so that I can add accessibility-related tags. I believe we should not keep a bug in Joomla core just to respect some personal preferences and force everyone to use overrides, whereas we can make a perfect/ working product.

avatar hans2103
hans2103 - comment - 11 Sep 2023

Although this pull request might solve a current bug, it should not create another.
Do not implement aria attributes when semantic html is suitable.

Why change the HTML when only changing the classNames would be enough?

when columns > 1

  • the className list-group set in <ul> should be changed into row mod-list
  • the classNames list-group-item and list-group-item-action set in <li> should be changed into col

this will result in the following view:

Scherm­afbeelding 2023-09-11 om 17 17 15

The borders around each item are gone, but the semantics are kept.
@nasirkhan Might this be a solution ?

avatar brianteeman
brianteeman - comment - 11 Sep 2023

Not 100% certain on the classes but the general principle of @hans2103 is the correct solution

avatar nasirkhan
nasirkhan - comment - 11 Sep 2023

@hans2103
Yours one could be a solution.

You want to show list items as a grid by the row-column CSS classes which is good to use with divs (as per Bootstrap doc). This list does not look like a list, visually, and it does not have list-related classes and you want to remove the list-group classes as well. And these li are showing h3 within it. So why are you using li for?

You said you want to use li to get the column count for screen readers, whereas the column count value is available on that page. If accessibility is the concern aria-colcount and aria-rowcount is the perfect solution for every browser-based and custom screen reader application.

Using ul-li and displaying it as a grid might not be a good example of semantic HTML either.

Add a Comment

Login with GitHub to post a comment