Skip to content
This repository was archived by the owner on Jan 25, 2021. It is now read-only.

Frontend search enhancements#31

Closed
richard67 wants to merge 10 commits intodevelopmentfrom
4.0-dev-frontend-search
Closed

Frontend search enhancements#31
richard67 wants to merge 10 commits intodevelopmentfrom
4.0-dev-frontend-search

Conversation

@richard67
Copy link
Member

@richard67 richard67 commented Aug 18, 2020

Pull Request for Issue #20 .

Summary of Changes

This PR is to collect, discuss and text the enhancements of the frontend search as defined in isssue #20 .

Requirements:

  • with label and / or icon
  • in extended form
  • as a component
  • With search results
  • on different places

Testing Instructions

Checkout the branch, or apply the changes of this branch to a current 4.0-dev branch or recent nightly of the CMS.

You can do that with patchtester 4:

patchtester-cassiopeia

(to be completed)

Expected result

(to be completed)

Actual result

(to be completed)

Documentation Changes Required

To be defined.

@richard67 richard67 marked this pull request as draft August 18, 2020 17:13
@richard67
Copy link
Member Author

Changed back to draft so we don't accidently merge it.

Comment on lines 30 to 32
<legend class="com-finder__search-legend sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_FORM_LEGEND'); ?>
</legend>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<legend class="com-finder__search-legend sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_FORM_LEGEND'); ?>
</legend>
<legend class="com-finder__search-legend sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_FORM_LEGEND'); ?>
</legend>

Please use tabs for indentation.

Comment on lines 56 to 58
<legend class="com-finder__search-advanced sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_ADVANCED_LEGEND'); ?>
</legend>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<legend class="com-finder__search-advanced sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_ADVANCED_LEGEND'); ?>
</legend>
<legend class="com-finder__search-advanced sr-only">
<?php echo Text::_('COM_FINDER_SEARCH_ADVANCED_LEGEND'); ?>
</legend>

- for styling it is much easier to style a list item then the combination of dt and dd
- screen readers can read out the number of list items in the ordered list
@hans2103
Copy link
Collaborator

DL => OL conversion
DL was implemented in joomla/joomla-cms#20572 by @Hackwar

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

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

@hans2103 hans2103 marked this pull request as ready for review August 27, 2020 06:30
@hans2103
Copy link
Collaborator

top right in Github: At least 6 approving reviews are required to merge this pull request.

@richard67
Copy link
Member Author

top right in Github: At least 6 approving reviews are required to merge this pull request.

That's not really relevant now since this PR here is just for development and maintenance but will not really be merged into the 4.0-dev branch.

@richard67 richard67 changed the base branch from 4.0-dev to development August 30, 2020 11:12
@richard67
Copy link
Member Author

I think this should be made as separate PR for the CMS, because it doesn't depend on or interact with our other developments here and touch the admin area. @hans2103 As the code originally comes from you: Can you make such PR for the 4.0-dev branch of the CMS?

@chmst
Copy link
Collaborator

chmst commented Aug 30, 2020

Please .... And ask the JAT team for a test.

@brianteeman
Copy link
Contributor

There are quite a few changes in this PR that are not correct and definitely nothing to do with the template. Please lets not repeat the mistakes of the past and leave non-template changes to the main repo

@hans2103
Copy link
Collaborator

hans2103 commented Aug 30, 2020

Please .... And ask the JAT team for a test.

@chmst how to trigger them? Besides ask them via Glip.

There are quite a few changes in this PR that are not correct ...

@brianteeman Can you be more specific. I will fix them.

... and definitely nothing to do with the template.

@brianteeman Planning to create multiple small PR's for joomla-cms repo

@brianteeman
Copy link
Contributor

@brianteeman Can you be more specific. I will fix them.

When the PR is in the correct place I will

@hans2103
Copy link
Collaborator

created PR joomla/joomla-cms#30528 to solve the legend issue.

@hans2103
Copy link
Collaborator

hans2103 commented Sep 1, 2020

created PR joomla/joomla-cms#30534 to solve styling of search results

@hans2103
Copy link
Collaborator

hans2103 commented Sep 1, 2020

created PR joomla/joomla-cms#30535 to add statement of search results

@hans2103
Copy link
Collaborator

hans2103 commented Sep 1, 2020

All changes of this PR have been converted to three separate Joomla/Joomla-CMS pull requests.
Closing this PR

@hans2103 hans2103 closed this Sep 1, 2020
@brianteeman
Copy link
Contributor

Thank you

@richard67 richard67 deleted the 4.0-dev-frontend-search branch September 2, 2020 11:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments