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

Mobile Menu: improve UX/UI of the mobile menu #10229

Merged
merged 3 commits into from Sep 20, 2018

Conversation

Projects
None yet
7 participants
@CaptainYouz
Contributor

CaptainYouz commented Aug 31, 2018

Questions Answers
Branch? develop
Description? This PR improve the UI of the mobile menu.
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? #9909
How to test? Test mobile version of the FO.

This change is Reviewable

@prestonBot prestonBot added the develop label Aug 31, 2018

@CaptainYouz

This comment has been minimized.

Show comment
Hide comment
@CaptainYouz

CaptainYouz Aug 31, 2018

Contributor

All visual changes approved by @TristanLDD

Contributor

CaptainYouz commented Aug 31, 2018

All visual changes approved by @TristanLDD

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Sep 11, 2018

Member

No need to port on StarterTheme. However, we need a rebase here.

Member

Quetzacoalt91 commented Sep 11, 2018

No need to port on StarterTheme. However, we need a rebase here.

@CaptainYouz

This comment has been minimized.

Show comment
Hide comment
@CaptainYouz

CaptainYouz Sep 11, 2018

Contributor

@Quetzacoalt91 rebase done

Contributor

CaptainYouz commented Sep 11, 2018

@Quetzacoalt91 rebase done

$('#footer').show();
}
$('#header').toggleClass('is-open');
if ($('#mobile_top_menu_wrapper').is(":visible")) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Sep 11, 2018

Contributor

I know everyone hate me for that but, use single quote instead of " :trollface:

@PierreRambaud

PierreRambaud Sep 11, 2018

Contributor

I know everyone hate me for that but, use single quote instead of " :trollface:

@mbadrani

This comment has been minimized.

Show comment
Hide comment
@mbadrani

mbadrani Sep 20, 2018

Contributor

@colinegin what do you think about this PR ?

Contributor

mbadrani commented Sep 20, 2018

@colinegin what do you think about this PR ?

@eternoendless eternoendless changed the title from FO | Mobile Menu: improve UX/UI of the mobile menu to Mobile Menu: improve UX/UI of the mobile menu Sep 20, 2018

@mbadrani mbadrani added QA ✔️ and removed waiting for PM labels Sep 20, 2018

@mickaelandrieu mickaelandrieu merged commit 7b212ca into PrestaShop:develop Sep 20, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Sep 20, 2018

vertical-align: middle;
width: 200px;
margin: 0 auto;
padding-top: 11px;

This comment has been minimized.

@rdy4ever

rdy4ever Sep 20, 2018

Contributor

This could create a problem. Not everyone is using a logo with the same dimensions as the default one and the 11px top padding does seem arbitrary to me.

@rdy4ever

rdy4ever Sep 20, 2018

Contributor

This could create a problem. Not everyone is using a logo with the same dimensions as the default one and the 11px top padding does seem arbitrary to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment