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

Allow to display a full width menu #6691

Merged
merged 1 commit into from Oct 19, 2016

Conversation

Projects
None yet
9 participants
@prestamodule
Contributor

prestamodule commented Oct 14, 2016

Questions Answers
Branch? develop
Description? Allow to display a full width menu, actually, that is limited to a 10 columns menu.
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? register any module to displayNavFullWidth and use it to display content
@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Oct 14, 2016

Contributor

Missing sql update into 1.7.0.0.sql nop ?

INSERT INTO PREFIX_hook(name,title,description,position) VALUES ('displayNavFullWidth', 'Navigation', 'This hook displays full width navigation menu at the top of your pages', '1');

Contributor

aleeks commented Oct 14, 2016

Missing sql update into 1.7.0.0.sql nop ?

INSERT INTO PREFIX_hook(name,title,description,position) VALUES ('displayNavFullWidth', 'Navigation', 'This hook displays full width navigation menu at the top of your pages', '1');

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 14, 2016

Contributor

@aleeks You're right, fixed.

Contributor

prestamodule commented Oct 14, 2016

@aleeks You're right, fixed.

@firstred

This comment has been minimized.

Show comment
Hide comment
@firstred

firstred Oct 15, 2016

Contributor

Almost same PR as #6500

Contributor

firstred commented Oct 15, 2016

Almost same PR as #6500

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

Hi @firstred this is not the same PR, almost yes, but the PR #6500 include the menu into a .col-xs-12 div, which is not full width at all as we are into a .container

Contributor

prestamodule commented Oct 17, 2016

Hi @firstred this is not the same PR, almost yes, but the PR #6500 include the menu into a .col-xs-12 div, which is not full width at all as we are into a .container

@aleeks

This comment has been minimized.

Show comment
Hide comment
@aleeks

aleeks Oct 17, 2016

Contributor

Hello @prestamodule

After concerting with the team, we think that it's not up to the module to make the html markup but at the theme creator, so, we find the solution of xs-12 better.

Moreover, I think your commit messages don't respect our standards : -SQL:..., it's FO: ..., BO: ..., etc..
If you think we don't misunderstood your idea, please let us know why you make this choice. (no div, etc).

For the moment, I'm closing your PR.
Regards

Contributor

aleeks commented Oct 17, 2016

Hello @prestamodule

After concerting with the team, we think that it's not up to the module to make the html markup but at the theme creator, so, we find the solution of xs-12 better.

Moreover, I think your commit messages don't respect our standards : -SQL:..., it's FO: ..., BO: ..., etc..
If you think we don't misunderstood your idea, please let us know why you make this choice. (no div, etc).

For the moment, I'm closing your PR.
Regards

@aleeks aleeks closed this Oct 17, 2016

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

Hi @aleeks
Here, we think that it's up to the module to make the html code to be compatible with any theme the merchant is using. This is why we create the right structure depending on configuration option integrated into our module.
This is very strange that again, the team changed their position regarding this.
We were agreed on this new "hook" last time we met the team @djfm @xBorderie

For now, we do only want to add this new hook in order to make it possible to have a FULL WIDTH menu, no matter the theme you are using, classic or not.
If nobody use this hook, it will not change the structure at all.
If the user want a full width menu, user can use this new hook, and any addon can hook on it.
If the user want a col-10 menu, he can use the displayNav hook.

Contributor

prestamodule commented Oct 17, 2016

Hi @aleeks
Here, we think that it's up to the module to make the html code to be compatible with any theme the merchant is using. This is why we create the right structure depending on configuration option integrated into our module.
This is very strange that again, the team changed their position regarding this.
We were agreed on this new "hook" last time we met the team @djfm @xBorderie

For now, we do only want to add this new hook in order to make it possible to have a FULL WIDTH menu, no matter the theme you are using, classic or not.
If nobody use this hook, it will not change the structure at all.
If the user want a full width menu, user can use this new hook, and any addon can hook on it.
If the user want a col-10 menu, he can use the displayNav hook.

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

Hi @aleeks

i think that you didn't get a point of this pull request, of course it's up to the module to set markup - if it's needed, we can't have situation where we need to ask third party company to edit their theme to be compatible with our module, come on!

Contributor

kpodemski commented Oct 17, 2016

Hi @aleeks

i think that you didn't get a point of this pull request, of course it's up to the module to set markup - if it's needed, we can't have situation where we need to ask third party company to edit their theme to be compatible with our module, come on!

@julienbourdeau

This comment has been minimized.

Show comment
Hide comment
@julienbourdeau

julienbourdeau Oct 17, 2016

Contributor

@prestamodule By merging this PR #6500 you will get a hook for your module. What's the problem with #6500?

By full width, do you mean full screen size even if the theme is 960px wide?

Contributor

julienbourdeau commented Oct 17, 2016

@prestamodule By merging this PR #6500 you will get a hook for your module. What's the problem with #6500?

By full width, do you mean full screen size even if the theme is 960px wide?

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

@julienbourdeau full width wrapper is needed in some cases, full width - not bootstrap full width :D

i'm in the same situation as @prestamodule in one of my modules so i'm with him on this one

Contributor

kpodemski commented Oct 17, 2016

@julienbourdeau full width wrapper is needed in some cases, full width - not bootstrap full width :D

i'm in the same situation as @prestamodule in one of my modules so i'm with him on this one

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

+1 @kpodemski
We mean full screen size, not full "bootstrap container" width.

Contributor

prestamodule commented Oct 17, 2016

+1 @kpodemski
We mean full screen size, not full "bootstrap container" width.

@julienbourdeau

This comment has been minimized.

Show comment
Hide comment
@julienbourdeau

julienbourdeau Oct 17, 2016

Contributor

That doesn't make any sense to me...

Why would you want a menu that is the size of the screen if you're using a boxed theme?!

Basically, this is what you want to achieve:
capture d ecran 2016-10-17 16 41 01

Contributor

julienbourdeau commented Oct 17, 2016

That doesn't make any sense to me...

Why would you want a menu that is the size of the screen if you're using a boxed theme?!

Basically, this is what you want to achieve:
capture d ecran 2016-10-17 16 41 01

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

Isn't is good enough that it does make sense to the two module/theme developers? lol

yes, this is what we want to achieve, thanks to this we can create full width sticky menu, we can set background on the full width of the page etc.

Contributor

kpodemski commented Oct 17, 2016

Isn't is good enough that it does make sense to the two module/theme developers? lol

yes, this is what we want to achieve, thanks to this we can create full width sticky menu, we can set background on the full width of the page etc.

@maximebiloe

This comment has been minimized.

Show comment
Hide comment
@maximebiloe

maximebiloe Oct 17, 2016

Contributor

All the theme developers may not put this hook outside any container. So, even if we merge the PR, we just have the guarantee it will on Classic. What about other themes?

Contributor

maximebiloe commented Oct 17, 2016

All the theme developers may not put this hook outside any container. So, even if we merge the PR, we just have the guarantee it will on Classic. What about other themes?

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

Doesn't matter, our job is to make our modules fully compatbile with default theme and try to be fully compatible with all the others

Contributor

kpodemski commented Oct 17, 2016

Doesn't matter, our job is to make our modules fully compatbile with default theme and try to be fully compatible with all the others

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

What about if a theme does not include displayNav ?
It can be part of validation rules.

Contributor

prestamodule commented Oct 17, 2016

What about if a theme does not include displayNav ?
It can be part of validation rules.

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

@maximebiloe There a lot of themes which doesn't respect PrestaShop coding rules.
We (as module editor) take time to explain to our customers how their theme is bad. We have to explain too PrestaShop inc. ask module editor to make their module works out of box for the default theme.

BUT, I think you have to listen your contributors.

You know we are selling the most popular menu for PrestaShop since almost 5 years.
Our customers want a FULL WIDTH menu. This is the point.

Your theme does NOT let them to make they want.

Please reconsider our PR
Here is a screenshot of what they need

Thanks
Steph
capture d ecran 2016-10-17 a 16 52 16

Contributor

prestamodule commented Oct 17, 2016

@maximebiloe There a lot of themes which doesn't respect PrestaShop coding rules.
We (as module editor) take time to explain to our customers how their theme is bad. We have to explain too PrestaShop inc. ask module editor to make their module works out of box for the default theme.

BUT, I think you have to listen your contributors.

You know we are selling the most popular menu for PrestaShop since almost 5 years.
Our customers want a FULL WIDTH menu. This is the point.

Your theme does NOT let them to make they want.

Please reconsider our PR
Here is a screenshot of what they need

Thanks
Steph
capture d ecran 2016-10-17 a 16 52 16

@julienbourdeau

This comment has been minimized.

Show comment
Hide comment
@julienbourdeau

julienbourdeau Oct 17, 2016

Contributor

Looking at your screenshot, I see a menu which is inside a container.

I agree with the fact that header should embed more hooks, and maybe a full width one for example. But I don't get the point of having one outside any container. :/

So on your screenshot you have markup .container>.row>.col-xs-12 in your module template. Hence what happens if the theme is actually full width? (.container-fluid)

Contributor

julienbourdeau commented Oct 17, 2016

Looking at your screenshot, I see a menu which is inside a container.

I agree with the fact that header should embed more hooks, and maybe a full width one for example. But I don't get the point of having one outside any container. :/

So on your screenshot you have markup .container>.row>.col-xs-12 in your module template. Hence what happens if the theme is actually full width? (.container-fluid)

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

Looking at your screenshot, I see a menu which is inside a container.

@julienbourdeau menu is not only the part where you see links

really, i'm surprised that we need to have discussion for something that simple and obvious

Contributor

kpodemski commented Oct 17, 2016

Looking at your screenshot, I see a menu which is inside a container.

@julienbourdeau menu is not only the part where you see links

really, i'm surprised that we need to have discussion for something that simple and obvious

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 17, 2016

Contributor

This is a theme issue, just follow the Bootstrap 4 guidelines and if the theme need a full width navbar, the hook will be on a container-fluid instead of a simple container.
If we follow this process, why not add a displayNavCol6, displayNavCol8, …

Our theme, I agree, only allow a fixed container.

We (as module editor) take time to explain to our customers how their theme is bad.

I understand, but even with any kind of solution the problem will be the same:

  • hook missing,
  • hook missplaced,
  • hook in a container div,
  • etc.

So, there is actually two solutions:

The first one is really simple and will work:
You use the bootstrap solution used to move the navigation between the mobile and desktop view to move the navigation where you want.
See https://github.com/PrestaShop/PrestaShop/blob/develop/themes/classic/_dev/js/responsive.js#L17

The second:
I agree to merge a hook "afterHeader" or another hook of this kind: more generic and avoid two hooks for the same thing. (In this case: just before the </header>)
BUT, if you do this: you also have to create the SAME script from the first solution to move the menu from the desktop view to the mobile view.

Contributor

Shudrum commented Oct 17, 2016

This is a theme issue, just follow the Bootstrap 4 guidelines and if the theme need a full width navbar, the hook will be on a container-fluid instead of a simple container.
If we follow this process, why not add a displayNavCol6, displayNavCol8, …

Our theme, I agree, only allow a fixed container.

We (as module editor) take time to explain to our customers how their theme is bad.

I understand, but even with any kind of solution the problem will be the same:

  • hook missing,
  • hook missplaced,
  • hook in a container div,
  • etc.

So, there is actually two solutions:

The first one is really simple and will work:
You use the bootstrap solution used to move the navigation between the mobile and desktop view to move the navigation where you want.
See https://github.com/PrestaShop/PrestaShop/blob/develop/themes/classic/_dev/js/responsive.js#L17

The second:
I agree to merge a hook "afterHeader" or another hook of this kind: more generic and avoid two hooks for the same thing. (In this case: just before the </header>)
BUT, if you do this: you also have to create the SAME script from the first solution to move the menu from the desktop view to the mobile view.

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

@julienbourdeau
If the nav hook is inside a container defined by the theme itself we will never be able to make a full width menu :-) We will be stuck in the center column !

This is why I think that the DOM structure must be done by the module itself.

Our module will generate the following :

  • a div (full width, a nice background customizable color, gradient or image)

-- a container (responsive, centered, with an alternative background color etc...)

--- a row

---- a list of tabs

So, even is the theme is using container-fluid this doesn't change anything, as our module will purpose an option to manage this case and embed its own CSS generator.

Hope this is more clear for you.

If not, please ask FM. We already talk about this last spring, and he was ok with us to make this happen. I just asked us to make this PR. This is why I don't understand why you changed your mind since.

Contributor

prestamodule commented Oct 17, 2016

@julienbourdeau
If the nav hook is inside a container defined by the theme itself we will never be able to make a full width menu :-) We will be stuck in the center column !

This is why I think that the DOM structure must be done by the module itself.

Our module will generate the following :

  • a div (full width, a nice background customizable color, gradient or image)

-- a container (responsive, centered, with an alternative background color etc...)

--- a row

---- a list of tabs

So, even is the theme is using container-fluid this doesn't change anything, as our module will purpose an option to manage this case and embed its own CSS generator.

Hope this is more clear for you.

If not, please ask FM. We already talk about this last spring, and he was ok with us to make this happen. I just asked us to make this PR. This is why I don't understand why you changed your mind since.

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

@Shudrum

  1. container-fluid would not force full-width if its already inside container
  2. moving html with js is a really bad solution, most of the time its causing FOUC-like effect with moving elements

Why do we need more generic hook if we already have displayNav1 and displayNav2 which doesn't make sens to any developer but author of the classic? :D

I agree to merge a hook "afterHeader" or another hook of this kind: more generic and avoid two hooks for the same thing. (In this case: just before the )

i don't agree, this hook probably would never be used for something different than the navigation

Contributor

kpodemski commented Oct 17, 2016

@Shudrum

  1. container-fluid would not force full-width if its already inside container
  2. moving html with js is a really bad solution, most of the time its causing FOUC-like effect with moving elements

Why do we need more generic hook if we already have displayNav1 and displayNav2 which doesn't make sens to any developer but author of the classic? :D

I agree to merge a hook "afterHeader" or another hook of this kind: more generic and avoid two hooks for the same thing. (In this case: just before the )

i don't agree, this hook probably would never be used for something different than the navigation

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 17, 2016

Contributor

If there is some errors done in the past: we have to continue?

I was also against the displayNav1 and 2. Originally named left and right, but not so generic. With 1 and 2, the theme creators can play with the position of some elements. (And I still dont like the nav on this name).

And, as I've said: you cannot be sure that the hook will be placed on a container-fluid or another kind of place where you will be able to use a fluid container.
It is still a theme issue…

Contributor

Shudrum commented Oct 17, 2016

If there is some errors done in the past: we have to continue?

I was also against the displayNav1 and 2. Originally named left and right, but not so generic. With 1 and 2, the theme creators can play with the position of some elements. (And I still dont like the nav on this name).

And, as I've said: you cannot be sure that the hook will be placed on a container-fluid or another kind of place where you will be able to use a fluid container.
It is still a theme issue…

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

And, as I've said: you cannot be sure that the hook will be placed on a container-fluid or another kind of place where you will be able to use a fluid container.
It is still a theme issue…

we don't need to have .container-fluid, we need to have hook outside any other .container, that's it, leave us the rest, i think that we know what we're doing, especially author of the #1 navigation module on Addons

If there is some errors done in the past: we have to continue?
I was also against the displayNav1 and 2. Originally named left and right, but not so generic. With 1 and 2, the theme creators can play with the position of some elements. (And I still dont like the nav on this name).

The problem is when someone will move 2 before 1. But maybe lets focus on what we really need, and this is hook outside container.

I do not want to be rude ... but I guess we know what we need? This is how the proposing of new hooks would look like? "We know better"?

Contributor

kpodemski commented Oct 17, 2016

And, as I've said: you cannot be sure that the hook will be placed on a container-fluid or another kind of place where you will be able to use a fluid container.
It is still a theme issue…

we don't need to have .container-fluid, we need to have hook outside any other .container, that's it, leave us the rest, i think that we know what we're doing, especially author of the #1 navigation module on Addons

If there is some errors done in the past: we have to continue?
I was also against the displayNav1 and 2. Originally named left and right, but not so generic. With 1 and 2, the theme creators can play with the position of some elements. (And I still dont like the nav on this name).

The problem is when someone will move 2 before 1. But maybe lets focus on what we really need, and this is hook outside container.

I do not want to be rude ... but I guess we know what we need? This is how the proposing of new hooks would look like? "We know better"?

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 17, 2016

Contributor

For remind : PrestaShop Addons team must validate templates no ?
If a theme editor put the hookFooter on top, I hope its theme will be declined !

The classic theme must be the "best practice" one.
This is like a warranty for module editors. If not, we will never be able to continue to provide a free support :-(

Contributor

prestamodule commented Oct 17, 2016

For remind : PrestaShop Addons team must validate templates no ?
If a theme editor put the hookFooter on top, I hope its theme will be declined !

The classic theme must be the "best practice" one.
This is like a warranty for module editors. If not, we will never be able to continue to provide a free support :-(

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 17, 2016

Contributor

For remind : PrestaShop Addons team must validate templates no ?
If a theme editor put the hookFooter on top, I hope its theme will be declined !

i think that it doesn't matter but as i said before, we should be fully compatible with Classic, right? Other themes should respect what Classic offers in standard, it was like this in 1.6, i hope it would be the same for 1.7 - but StarterTheme may be the problem here

Contributor

kpodemski commented Oct 17, 2016

For remind : PrestaShop Addons team must validate templates no ?
If a theme editor put the hookFooter on top, I hope its theme will be declined !

i think that it doesn't matter but as i said before, we should be fully compatible with Classic, right? Other themes should respect what Classic offers in standard, it was like this in 1.6, i hope it would be the same for 1.7 - but StarterTheme may be the problem here

@PrestaEdit

This comment has been minimized.

Show comment
Hide comment
@PrestaEdit

PrestaEdit Oct 17, 2016

Contributor

I think that the theme issue will be there if the theme handle the hook inside a container.

You will never be sure that the theme manage all the hooks you have inside the Classic (default) one. It's yet happen with the default-bootstrap and many themes sold on Addons Marketplace.

But, what ? We need to not make proposal about new hooks on the FO because there is no native module on it ? Do you know how many hooks is there but still not used by a native module ? (but so usefull for third one, for sure !).

It's not a move of an existing hook (like the displayTop) but a new one. A named "FullWidth". The theme will need to have it according to a full widht space (as is it inside this PR) and the module editor need to use it if he want to make the module (not only a menu !) full width. Or, it will use the displayTop.

I'm not sure to manage the problem to have this new hook as it.

Contributor

PrestaEdit commented Oct 17, 2016

I think that the theme issue will be there if the theme handle the hook inside a container.

You will never be sure that the theme manage all the hooks you have inside the Classic (default) one. It's yet happen with the default-bootstrap and many themes sold on Addons Marketplace.

But, what ? We need to not make proposal about new hooks on the FO because there is no native module on it ? Do you know how many hooks is there but still not used by a native module ? (but so usefull for third one, for sure !).

It's not a move of an existing hook (like the displayTop) but a new one. A named "FullWidth". The theme will need to have it according to a full widht space (as is it inside this PR) and the module editor need to use it if he want to make the module (not only a menu !) full width. Or, it will use the displayTop.

I'm not sure to manage the problem to have this new hook as it.

@PrestaEdit

This comment has been minimized.

Show comment
Hide comment
@PrestaEdit

PrestaEdit Oct 17, 2016

Contributor

The only thing I will agree with you about this hook is the name. I think that a displayFullWidthTop (or like it) will be better than "displayNav...", because it's just not related to the "header_nav" but more to the "header_top" block. :)

Contributor

PrestaEdit commented Oct 17, 2016

The only thing I will agree with you about this hook is the name. I think that a displayFullWidthTop (or like it) will be better than "displayNav...", because it's just not related to the "header_nav" but more to the "header_top" block. :)

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Oct 18, 2016

Contributor

...and?

Contributor

kpodemski commented Oct 18, 2016

...and?

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 18, 2016

Contributor

Long thinking in my side about this pull request.

You guys seems really motivated to get this, and I think I have understand a main problem with our theme system and the lack of modularity … You and some people of the team make me changed my mind. I'm pretty sure we will talk soon about many theme adding this hook on a container or simply removing it, and maybe many other problems. But … even if we don't merge this hook we will get the same problems.

So if it help you as much as you say… I agree to merge it if one of you add this hook to the starter theme, link the pull request here, and rebase this one to match our commit norms.

Contributor

Shudrum commented Oct 18, 2016

Long thinking in my side about this pull request.

You guys seems really motivated to get this, and I think I have understand a main problem with our theme system and the lack of modularity … You and some people of the team make me changed my mind. I'm pretty sure we will talk soon about many theme adding this hook on a container or simply removing it, and maybe many other problems. But … even if we don't merge this hook we will get the same problems.

So if it help you as much as you say… I agree to merge it if one of you add this hook to the starter theme, link the pull request here, and rebase this one to match our commit norms.

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 18, 2016

Contributor

@Shudrum done, also made the PR on StarterTheme repo

Contributor

prestamodule commented Oct 18, 2016

@Shudrum done, also made the PR on StarterTheme repo

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 18, 2016

Contributor

Still not the right commit norms, sorry.

Contributor

Shudrum commented Oct 18, 2016

Still not the right commit norms, sorry.

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 18, 2016

Contributor

And you commited two times the same thing:

Maybe a rebase fail? Maybe you added the Starter Theme commit here also?

Note: StarterTheme PR: (PrestaShop/StarterTheme#139)

Contributor

Shudrum commented Oct 18, 2016

And you commited two times the same thing:

Maybe a rebase fail? Maybe you added the Starter Theme commit here also?

Note: StarterTheme PR: (PrestaShop/StarterTheme#139)

@prestamodule

This comment has been minimized.

Show comment
Hide comment
@prestamodule

prestamodule Oct 18, 2016

Contributor

@Shudrum now ?

Contributor

prestamodule commented Oct 18, 2016

@Shudrum now ?

@Shudrum

This comment has been minimized.

Show comment
Hide comment
@Shudrum

Shudrum Oct 18, 2016

Contributor

Perfect!

I'm waiting for the tests and merge!

Contributor

Shudrum commented Oct 18, 2016

Perfect!

I'm waiting for the tests and merge!

@Shudrum Shudrum merged commit 2108297 into PrestaShop:develop Oct 19, 2016

1 check passed

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