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

Fix search bar on header when it moves from hook and improve header structure #2 #8845

Merged
merged 4 commits into from Mar 19, 2018

Conversation

@sLorenzini
Copy link
Contributor

commented Mar 13, 2018

Questions Answers
Branch? 1.7.3.x
Description? Search bar was broken when we add it on displayNavX. Structure of header have been improved to be more flexible
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-4306
How to test? Add search bar on displayNav1 and/or displayNav2 hook

Important guidelines


This change is Reviewable

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

:lgtm:


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Thank you @sLorenzini

@LittleBigDev LittleBigDev added this to the 1.7.3.1 milestone Mar 14, 2018

See comments

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

:lgtm_cancel:

See comments

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


themes/classic/_dev/css/theme.scss, line 123 at r1 (raw file):

    }
    .search-widget {
      margin-top: 3px;

Please use rem unit to keep consistent with Bootstrap


themes/classic/_dev/css/theme.scss, line 241 at r1 (raw file):

      .search-widget {
       float: none;
       width: 250px;

Please use rem unit to keep consistent with Bootstrap


themes/classic/_dev/css/theme.scss, line 271 at r1 (raw file):

    width: 100%;
    display: block;
    max-width: 250px;

Please use rem unit to keep consistent with Bootstrap


themes/classic/_dev/css/components/search-widget.scss, line 54 at r1 (raw file):

@include media-breakpoint-up(md) {
  .search-widget {
    min-width: 250px;

Please use rem unit to keep consistent with Bootstrap


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

px unit should be avoided. Please stay consistent with Bootstrap, using rem unit.

@sLorenzini

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2018

@LittleBigDev, changes done. Thanks

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

:lgtm:


Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@LittleBigDev
Copy link
Contributor

left a comment

Thank you @sLorenzini

@LittleBigDev

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Thank you @sLorenzini

@eternoendless eternoendless merged commit 12eaf90 into PrestaShop:1.7.3.x Mar 19, 2018

2 of 3 checks passed

code-review/reviewable 4 files left (LittleBigDev)
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.