Skip to content

Comments

[4.0] [a11y] add statement on found results#30535

Merged
wilsonge merged 11 commits intojoomla:4.0-devfrom
hans2103:feature/4.0-dev-a11y--com_finder-add-statement-of-search-results
Sep 2, 2020
Merged

[4.0] [a11y] add statement on found results#30535
wilsonge merged 11 commits intojoomla:4.0-devfrom
hans2103:feature/4.0-dev-a11y--com_finder-add-statement-of-search-results

Conversation

@hans2103
Copy link
Contributor

@hans2103 hans2103 commented Sep 1, 2020

Pull Request for Issue # .

Summary of Changes

With this PR I've added the total amount of search results and a link to the search results.
I am missing a statement about how many search results were found. I only get this information when no results are found.
When search results are found I would like to be able to click on it and jump directly to the results.

Testing Instructions

  • Search for blog and see the results.

Actual result BEFORE applying this Pull Request

Schermafbeelding 2020-08-18 om 15 18 52

Expected result AFTER applying this Pull Request

screenshot 1

When there are no results add a link to search again
Schermafbeelding 2020-08-18 om 15 58 50

screenshot 2

When there are results add a link to jump to the results. Just like a skip-link
Schermafbeelding 2020-08-18 om 15 57 57

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Sep 1, 2020
@hans2103 hans2103 changed the title add statement on found results [4.0] [a11y] add statement on found results Sep 1, 2020
@brianteeman
Copy link
Contributor

what about giving the message a role="alert" ? that way it should result in the number of results being announced immediately on page load

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

what about giving the message a role="alert" ? that way it should result in the number of results being announced immediately on page load

just wrote a new issue
#30536

@brianteeman I see you write role=alert while I was thinking about role=status
Which one is better?

@brianteeman
Copy link
Contributor

You cant use status as that is for live regions ie parts of the page that are updated
In our case the page is reloaded and not updated

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

thank you.

what about giving the message a role="alert" ?

Do you mean something like this:

$text = "no results OR one result OR x amount of results"
Factory::getApplication()->enqueueMessage($text, 'alert');

@brianteeman
Copy link
Contributor

Sorry I used the word message incorrectly - i think

You can just put the role on the existing element - you dont need to use the enqueMessage as that creates a message at the top of the page (doesnt it?)

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

@brianteeman I've added role=alert to the announcement of search results. It had to be added to the language strings.

COM_FINDER_QUERY_RESULTS="the following <a href=\"#search-result-list\" role=\"alert\">%s results were found</a>."
COM_FINDER_QUERY_RESULTS_0="<span role=\"alert\">no results were found</span>. <a href=\"#q\">Search again</a>."
COM_FINDER_QUERY_RESULTS_1="the following <span role=\"alert\">%s result was found</span>."

@brianteeman
Copy link
Contributor

thats not going to work and its also not good practice to put markup inside a language string

this should work (not tested as I'm on my phone) . replace div if needed with any block element

<?php // Display the explained search query. ?>
		<div role="alert">
			<?php echo Text::sprintf('COM_FINDER_QUERY_TOKEN_INTERPRETED', $this->explained, Text::plural('COM_FINDER_QUERY_RESULTS', $this->total)); ?>
		</div>

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

@brianteeman thank you for the feedback.
I've placed the text displayed in id="search-query-explained" in a paragraph element.

Assuming blog is required, the following 5 results were found.

<p role="alert">Assuming <span class="query-required"><span class="term">blog</span> is required</span>, the following <a href="#search-result-list">5 results were found</a>.</p>

@paternax
Copy link

paternax commented Sep 1, 2020

I have tested this item ✅ successfully on 6eca68f


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

@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

@Quy and @brianteeman thank you for your contributions. I did some tests with plural to prevent the double strings. But it failed on my test. Now it seems to work and that makes me happy.
I have applied the suggested commits from @Quy.

Copy link
Contributor Author

@hans2103 hans2103 left a comment

Choose a reason for hiding this comment

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

@Quy Thank you. How can the commit suggestions be applied to my PR?

@Quy
Copy link
Contributor

Quy commented Sep 1, 2020

@hans2103 There is a Commit suggestion button under the Suggested change code.

hans2103 and others added 2 commits September 1, 2020 21:16
Co-authored-by: Quy <quy@fluxbb.org>
Co-authored-by: Quy <quy@fluxbb.org>
@hans2103
Copy link
Contributor Author

hans2103 commented Sep 1, 2020

@Quy thank you.. I was trying to merge the suggestions through the overview of changed files. But that didn't stick.
Now it seems to work. Thank you

Co-authored-by: Quy <quy@fluxbb.org>
@paternax
Copy link

paternax commented Sep 1, 2020

I have tested this item ✅ successfully on 709eb8b


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

Co-authored-by: Quy <quy@fluxbb.org>
@ghost
Copy link

ghost commented Sep 2, 2020

I have tested this item ✅ successfully on f7c9996

Works as shown in "Expected result AFTER applying this Pull Request".


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

@paternax
Copy link

paternax commented Sep 2, 2020

I have tested this item ✅ successfully on f7c9996


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

@N6REJ N6REJ added the RTC This Pull Request is Ready To Commit label Sep 2, 2020
@wilsonge wilsonge merged commit 5e10825 into joomla:4.0-dev Sep 2, 2020
@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2020

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 2, 2020
@wilsonge wilsonge added this to the Joomla 4.0 milestone Sep 2, 2020
@hans2103 hans2103 deleted the feature/4.0-dev-a11y--com_finder-add-statement-of-search-results branch September 2, 2020 12:59
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

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants