[#13498] - [com_search] Refactoring + fixing results-highlighting
- Fixed in Code Base
- 5 Feb 2017
- Medium
- Build: staging
- # 13498
- Diff
- frankmayer:com_search-results-highlighting
User tests: Successful: Unsuccessful:
New issue:
Positioning of highlighting was off sometimes when special characters were used.
Also the css padding for highlighting feels 'unnatural'.
Please look below for example images showing the before and after the patch.
Summary of Changes
- [com_search] Refactoring + fixing results-highlighting
- Fixed css sitewide to display highlight more naturally.
Testing Instructions
- Do not apply patch. Do some searches using words with ascii and non-ascii.
- Apply patch. Check that the correct words are highlighted.
Documentation Changes Required
None
Note: It would probably be better to make the functionality of highlighting reusable in it's own class. It then could also be reused from other parts of Joomla itself or extensions. It would also allow us to extend its functionality.
I would do that, in another PR, if this one gets merged.
Examples:
Notice the wrong positioning of the highlighting in the text at some places and the unnatural padding on the highlight. Both issues are fixed in the image after this one, where the patch is applied.
The word to search for, in these examples, was: Österreich
(image showing the issues)
(image showing issues fixed)
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Front End Templates (site) |
| Labels |
Added:
?
|
||
I have tested this item
Very welcome patch :-)
Thank you
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
@genesisfan did this work without the patch? Could you post a screenshot without the patch?
@frankmayer with or without the patch, the search output stay the same. The non-ascii word is not highlighted.
@genesisfan Thank you for testing and finding this problem.
However, since the problem pre-existed and was not introduced by the patch, I would propose to create a separate issue for that. That way this specific improvement can be merged and a fix for the other issue can be worked on, after that.
I have tested this item
Thanks!
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
So can this be merged then? The unsuccessful human test is not related to the fix this PR is proposing. Since another issue has already be created for that, I think it would be better to merge this PR in order to avoid conflicts in future work for the other issue.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
I have tested this item
Issue confirmed,
Test successful - Works as described
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
I have tested this item
patch works as descripted.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.
zero-24
- | Milestone |
Added: |
||
| Status | Pending | ⇒ | Ready to Commit |
| Labels |
Added:
?
|
||
| Milestone |
Removed: |
||
| Status | Ready to Commit | ⇒ | Pending |
| Status | Pending | ⇒ | Ready to Commit |
wilsonge
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-02-05 22:26:36 |
| Closed_By | ⇒ | wilsonge | |
| Labels | |||
This is a really nice improvement. see picture below
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13498.