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

New searchbar design #12

Merged
merged 2 commits into from Jan 8, 2020
Merged

New searchbar design #12

merged 2 commits into from Jan 8, 2020

Conversation

@atomiix
Copy link

atomiix commented Oct 21, 2019

Questions Answers
Branch? dev
Description? New design for the search bar
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #13732
How to test? Checkout the module with this branch, search any product on the store, please check the search now meets #13732 requirements

Before:
Capture d’écran 2019-11-06 à 18 11 35

After:
Capture d’écran 2019-11-06 à 18 13 43

@atomiix atomiix force-pushed the atomiix:new-design branch from 9e3f9eb to f720c8f Oct 21, 2019
}
#search_widget form i {
position: absolute;
padding: 8px;

This comment has been minimized.

Copy link
@Progi1984

Progi1984 Oct 29, 2019

Please use rem rather than px


$.widget('prestashop.psBlockSearchAutocomplete', $.ui.autocomplete, {
_renderItem: function (ul, product) {
var $img = $('<div class="autocomplete-thumbnail">');

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member

var $img actually references a <div>

if (product.cover) {
$img.append('<img src="'+product.cover.bySize.small_default.url+'">')
} else {
$img.append('<img src="'+prestashop.urls.no_picture_image.bySize.small_default.url+'">')
}
Comment on lines 11 to 15

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member

Looks like this could be made simpler:

const coverImage = (product.cover) ? product.cover : prestashop.urls.no_picture_image;

return $('<li>')
  .append($('<a>'))
     .append($('<img>').attr('src', coverImage.bySize.small_default.url))
     .append(/* ... */)
<form method="get" action="{$search_controller_url}">
<input type="hidden" name="controller" value="search">
<i class="material-icons search">&#xE8B6;</i>
<input type="text" name="s" value="{$search_string}" placeholder="{l s='Search our catalog' d='Modules.Searchbar.Catalog'}" aria-label="{l s='Search' d='Shop.Theme.Catalog'}">

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member
Modules.Searchbar.Catalog

I don't think this domain is right, wdyt @LouiseBonnard ?

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Dec 11, 2019

Suggested change
<input type="text" name="s" value="{$search_string}" placeholder="{l s='Search our catalog' d='Modules.Searchbar.Catalog'}" aria-label="{l s='Search' d='Shop.Theme.Catalog'}">
<input type="text" name="s" value="{$search_string}" placeholder="{l s='Search our catalog' d='Shop.Theme.Catalog'}" aria-label="{l s='Search' d='Shop.Theme.Catalog'}">
<div id="search_widget" class="search-widgets" data-search-controller-url="{$search_controller_url}">
<form method="get" action="{$search_controller_url}">
<input type="hidden" name="controller" value="search">
<i class="material-icons search">&#xE8B6;</i>

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member

Remember to use aria-hidden="true" for icons so that they are hidden from screen readers.

Suggested change
<i class="material-icons search">&#xE8B6;</i>
<i class="material-icons search" aria-hidden="true">&#xE8B6;</i>

Also, you can change &#xE8B6; to the actual icon name from material icons (eg. "search")

ps_searchbar.tpl Show resolved Hide resolved
<input type="hidden" name="controller" value="search">
<i class="material-icons search">&#xE8B6;</i>
<input type="text" name="s" value="{$search_string}" placeholder="{l s='Search our catalog' d='Modules.Searchbar.Catalog'}" aria-label="{l s='Search' d='Shop.Theme.Catalog'}">
<i class="material-icons clear">&#xe14c;</i>

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member

Same comment as with the previous icon

if ($searchBox.val() !== "" && isMobile()) {
$clearButton.show();
} else {
$clearButton.hide();
}
Comment on lines 67 to 71

This comment has been minimized.

Copy link
@eternoendless

eternoendless Oct 29, 2019

Member

You can simplify using .toggle():

$clearButton.toggle($searchBox.val() !== "" && isMobile());
@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Nov 5, 2019

Would you please post a before/after screenshot? :)

@colinegin

This comment has been minimized.

Copy link

colinegin commented Dec 11, 2019

@atomiix do you know what is missing here to merge this PR ?

@atomiix

This comment has been minimized.

Copy link
Author

atomiix commented Dec 11, 2019

@atomiix do you know what is missing here to merge this PR ?

Yes, I have some modifications to do but I need @LouiseBonnard confirmation on the domain to use.

@colinegin

This comment has been minimized.

Copy link

colinegin commented Dec 18, 2019

@LouiseBonnard is it ok on your side now ?

@LouiseBonnard

This comment has been minimized.

Copy link

LouiseBonnard commented Dec 18, 2019

@colinegin, @atomiix, as soon as this commit is merged! cf. #12 (comment)

@eternoendless eternoendless requested a review from NeOMakinG Dec 23, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Jan 2, 2020

@atomiix
the search icon is missing
Capture d’écran 2020-01-02 à 16 14 07

@sarahdib sarahdib self-assigned this Jan 2, 2020
@atomiix

This comment has been minimized.

Copy link
Author

atomiix commented Jan 3, 2020

@atomiix
the search icon is missing
Capture d’écran 2020-01-02 à 16 14 07

@sarahdib I forgot to explain that you need to delete the module template override in the theme themes/classic/modules/ps_searchbar and clear the cache then

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jan 3, 2020
@Progi1984 Progi1984 merged commit b3cc679 into PrestaShop:dev Jan 8, 2020
@Progi1984

This comment has been minimized.

Copy link

Progi1984 commented Jan 8, 2020

Thanks @atomiix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.