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

[4.0] com_finder ul instead of dl for easier styling #30534

Conversation

hans2103
Copy link
Contributor

@hans2103 hans2103 commented Sep 1, 2020

Pull Request for Issue # .

Summary of Changes

quote from @Hackwar taken from a private message to find out reason of changing ul to dl

I used a DL because I considered that the syntactically correct structure. There is no deeper reasoning.

Changing it from DL back to UL makes it easier for frontend developers to style each search result.
Styling DT+DD is not as easy as styling a LI

Testing Instructions

  • Go to the search results.
    • using Joomla 4 with Blog Sample Data it will be the following url with blog as searched word.
      /index.php/component/finder/search?q=blog&Itemid=101&Itemid=101
  • Inspect element on the search results and see the HTML structure
  • Apply this PR, refresh the page
  • Inspect element on the search results and see the HTML structure

Actual result BEFORE applying this Pull Request

It is very hard to style dt+dd+dd+dd as a card. Try adding a background image, box-shadow and spacing between the individual search results.

Schermafbeelding 2020-09-01 om 08 56 15

Expected result AFTER applying this Pull Request

After applying this PR it is much easier to style individual search results.

Schermafbeelding 2020-09-01 om 08 55 27

Documentation Changes Required

@paternax
Copy link

paternax commented Sep 1, 2020

I have tested this item ✅ successfully on c9f4593

The code looks like described.

Perhaps a bit overstructured in the inner part of the li-element. There are three levels: header->h4->a for each result title and three times the same class: result-title(in li, header and h4)


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

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

Thank you for testing.

@paternax

Perhaps a bit overstructured in the inner part of the li-element. There are three levels: header->h4->a for each result title and three times the same class: result-title(in li, header and h4)

After merging this PR where DL will be replaced by UL I will create a new PR to simplify the HTML structure of a search result. I agree on the HTML structure.

@Quy
Copy link
Contributor

Quy commented Sep 1, 2020

I have tested this item ✅ successfully on c9f4593


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

@Quy
Copy link
Contributor

Quy commented Sep 1, 2020

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 1, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2020

Makes sense. But given it’s search results I assume ol is more logical than ul as they are ordered by search relevancy

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 2, 2020

@wilsonge can changing ul into ol be done in another PR?
The impact is bigger... since the search results can be stretched over multiple pages a <li start="number"> should be added to prevent the numbering start over on 1 on the second page and so on.

@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2020

Thats true. But still think it makes more sense to do it like that

@wilsonge wilsonge merged commit 08de156 into joomla:4.0-dev Sep 2, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 2, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2020

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Sep 2, 2020
@hans2103 hans2103 deleted the 4.0-dev-a11y--com_finder-ol-instead-of-dl-for-easier-styling branch September 2, 2020 09:13
@hans2103
Copy link
Contributor Author

hans2103 commented Sep 2, 2020

@wilsonge Created a draft PR to adjust individual search result #30550
Including using a ol instead of ul

dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Sep 29, 2020
…om_templates

* '4.0-dev' of github.com:joomla/joomla-cms: (70 commits)
  [4.0] Child templates consistency (joomla#30387)
  [4.0] favicon changes to support child templates (joomla#30388)
  [4.0] Update Readme for Api tests (joomla#30539)
  [4.0] [Multilingual Status module] Adding displaying a possible error if URL Language Code is empty (joomla#30537)
  [4.0] Display of horizontal mod_articles_news module (joomla#30527)
  [4.0] Useless installation lang strings (joomla#30568)
  [4.0] Numbers not digits (joomla#30559)
  [4.0] Accessibility plugin position (joomla#30552)
  [4.0] fix for inherit fields (joomla#30557)
  [4.0] Redundant words (joomla#30555)
  add missing legend to fieldset (joomla#30528)
  [4.0] [a11y] add statement on found results (joomla#30535)
  [4.0] com_finder ul instead of dl for easier styling (joomla#30534)
  [4.0] Messages/Alerts: using icons instead of text as heading (joomla#30516)
  [4.0] Increase API Test Coverage (joomla#26722)
  [4.0] Implementing display of password requirements for frontend (joomla#30473)
  [4.0] FieldsHelper: Choose a first available category  correctly (joomla#30268)
  Sort options (joomla#30531)
  Clear checkboxes on back button (joomla#30498)
  Update _icomoon.scss (joomla#30436)
  ...
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
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.

5 participants