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

Improve header BO design #8914

Merged
merged 18 commits into from Jun 5, 2018

Conversation

@sLorenzini
Copy link
Contributor

commented Apr 3, 2018

Questions Answers
Branch? 1.7.4.x
Description? Improve design on header BO
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/FF-169
How to test? Log in on the BO and take a look at the header

Important guidelines


This change is Reviewable

@sLorenzini sLorenzini force-pushed the sLorenzini:FF-169 branch from 6e00b25 to 721a53b Apr 5, 2018

@eternoendless eternoendless added this to the 1.7.4.0 milestone Apr 23, 2018

@eternoendless eternoendless force-pushed the sLorenzini:FF-169 branch from 721a53b to f29aa54 Apr 23, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

Reviewed 23 of 25 files at r1, 25 of 26 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


admin-dev/themes/default/sass/partials/_icons.sass, line 447 at r1 (raw file):

		@extend .icon-toggle-off

[class^="material-icons"]

This setting is too broad. It's breaking the new menu, and possibly other things too.

With it:
Screen Shot 2018-04-23 at 18.11.40.png

Without:
Screen Shot 2018-04-23 at 18.14.31.png


Comments from Reviewable

@eternoendless
Copy link
Member

left a comment

Some changes are needed

@sLorenzini sLorenzini force-pushed the sLorenzini:FF-169 branch from f29aa54 to d75b746 Apr 27, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

Reviewed 4 of 19 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


admin-dev/themes/default/sass/partials/_icons.sass, line 450 at r3 (raw file):

	font-size: 20px
	vertical-align: middle
	position: relative

Why not put all of this in _header.sass? I think that will be less risky


Comments from Reviewable

@sLorenzini sLorenzini force-pushed the sLorenzini:FF-169 branch from d75b746 to 499ccc6 Apr 27, 2018

@sLorenzini

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2018

@eternoendless done :)

@sLorenzini sLorenzini force-pushed the sLorenzini:FF-169 branch from 499ccc6 to 19bd9a1 Apr 27, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

:lgtm:


Reviewed 2 of 3 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@marionf

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

Following returns seen with @colinegin

  • Color & size of the "eye" logo, difference between legacy and symfony pages. We want to keep the legacy one.
  • Color & size of the "debug mode" logo, difference between legacy and symfony pages. We want to keep the legacy one. The dropdown is truncated.

capture du 2018-04-27 17-27-07

  • Maintenance mode displayed on legacy pages even if it's not enabled.
  • Color & size of the "Maintenance mode" logo, difference between legacy and symfony pages. We want to keep the legacy one. The tooltip is truncated on symfony pages.
  • Color & size of the "my account" logo, difference between legacy and symfony pages. We want to keep the legacy one.
  • Color & size of the "gamification" logo, difference between legacy and symfony pages. We want to keep the symfony one
  • Multistore menu: differences between legacy and symfony page. We want to keep the symfony one. With the same size of others and the text in #4e6167.
@@ -24,6 +24,7 @@
{* Logo *}
<i class="material-icons js-mobile-menu">menu</i>
<a id="header_logo" class="logo float-left" href="{$default_tab_link|escape:'html':'UTF-8'}"></a>
<span id="shop_version">{$ps_version}</span>

This comment has been minimized.

Copy link
@Quetzacoalt91

@sLorenzini sLorenzini force-pushed the sLorenzini:FF-169 branch from 3bfb3c2 to b521bbf May 3, 2018

@eternoendless eternoendless force-pushed the sLorenzini:FF-169 branch from abfe8d5 to a540c53 Jun 4, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

Rebased again

border-radius: 0
min-width: 17.75rem !important
box-shadow: 0 2px 2px 0 rgba(0, 0, 0, 0.1)
border: 1px solid #bbcdd2

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

Border color can be a variable because used many times.

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jun 4, 2018

Member

Yeah, I don't think it's really important right now though.

box-shadow: 0 2px 2px 0 rgba(0, 0, 0, 0.1)
border: 1px solid #bbcdd2
top: 40px
right: -100px !important

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

!important is bad, are you sure there is no other way?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jun 4, 2018

Member

I agree, either way don't worry too much about the default theme as it will die with 1.7

In this case, I think it was "important", because it was needed to to move an already aligned floating menu.

@@ -10,7 +10,10 @@
color: #363A41
padding: 0 0 0 210px

@media (max-width: 1024px)
.mobile &
top: $size-header-height + ($header-mobile-padding-y * 2)

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

Can you remove tabs and add spaces instead? (In an other commit ofc)

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jun 4, 2018

Member

I can, but not in this PR 😄
I'd like to upgrade to webpack 4, too

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

👍

{if isset($is_multishop)
&& $is_multishop
&& $shop_list
&& (

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

Something is weird here, maybe use parenthesis

&& (
    isset($multishop_context)
    && 
    (
        $multishop_context & Shop::CONTEXT_GROUP
        || $multishop_context & Shop::CONTEXT_SHOP
        || $multishop_context & Shop::CONTEXT_ALL
    )
  )}

or

&& (
    (isset($multishop_context) && $multishop_context & Shop::CONTEXT_GROUP) ||
    $multishop_context & Shop::CONTEXT_SHOP ||
    $multishop_context & Shop::CONTEXT_ALL
  )}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jun 4, 2018

Member

Actually I don't think this thing works as intended, but it's not the objective of this PR. I tried to, though.

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Jun 4, 2018

Contributor

In fact I don't understand what the if must do with mixed && and || :/

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jun 5, 2018

Member

Honestly... neither do I! We'll figure that out later

@marionf marionf added the QA ✔️ label Jun 4, 2018

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

Hello @eternoendless, if it's ready let's merge it.

Afaik this is the only blocking issue for the release :)

@eternoendless

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

Thanks for the reviews!

@eternoendless eternoendless merged commit dfaaaf1 into PrestaShop:1.7.4.x Jun 5, 2018

2 of 3 checks passed

code-review/reviewable 37 files, 7 discussions left (eternoendless, PierreRambaud, Quetzacoalt91, sLorenzini)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eternoendless eternoendless changed the title Clean Header BO design for merchant Improve header BO design Jun 5, 2018

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