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

Fixed List All Tags view #41440

Closed
wants to merge 1 commit into from

Conversation

nasirkhan
Copy link
Contributor

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

reflect changes of view with column count
@laoneo
Copy link
Member

laoneo commented Aug 26, 2023

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

@laoneo laoneo added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Aug 26, 2023
@nasirkhan
Copy link
Contributor Author

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.

@hans2103
Copy link
Contributor

hans2103 commented Sep 11, 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.

@laoneo
Copy link
Member

laoneo commented Sep 11, 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.

@laoneo laoneo closed this Sep 11, 2023
@laoneo laoneo removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Sep 11, 2023
@nasirkhan
Copy link
Contributor Author

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.

@hans2103
Copy link
Contributor

hans2103 commented Sep 11, 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 ?

@brianteeman
Copy link
Contributor

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

@nasirkhan
Copy link
Contributor Author

nasirkhan commented Sep 11, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants