-
Notifications
You must be signed in to change notification settings - Fork 80
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
Removed .env configuration #629
Conversation
I know that some variables will remain empty, but the build seems ok like this. Removing .env dependency will help us to ship hummingbird in prestashop core. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env configuration was added for a reason. 🤔 Ping @Oksydan @NeOMakinG
I know that there were some reasons ... but we can't find them. |
PR that introduced this, with discussion - #67 |
OK so it seems that you warned about this issue... |
@nicosomb .env file is being used by webpack-dev-server mostly to improve DX. Why would you like to get ride of this like this 🤔. |
@Oksydan I understand, but can you tell us how can we ship it in PrestaShop core please ? We want to add Hummingbird as a dependency in composer.json (see my PR here PrestaShop/PrestaShop#36198). So we have to know how to fill the .env file during the assets build process. |
Hmmmmm we could pass arguments in the compilation process. If the process is building a theme, we could simply not check if the const {
PORT: port = null,
PUBLIC_PATH: publicPath = null,
SERVER_ADDRESS: serverAddress = null,
SITE_URL: siteURL = null,
} = process.env; THB we do not need these values in the compilation process. It is only required for the development process. |
@Oksydan thank you for the details. I will improve my PR. |
76f0070
to
35ac6b6
Compare
@@ -176,7 +176,7 @@ | |||
{/block} | |||
</div> | |||
|
|||
{if $product.add_to_cart_url && $product.product_type !== 'combinations'} | |||
{if $product.add_to_cart_url} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this diff? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to this PR: #609
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that. But it seems that it breaks UI tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's an issue about UI tests then.
If we merge your diff, we reintroduce the "Add to cart" button for combinations products, and his buggy behaviour in that particular case 🤔
But anyway, I don't block your PR, just I prefer to warn you about this! 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is legit, add to cart url should be handled on product lazy array settings. For combination, it's availability depends on prestashop settings (enable add to cart button in listings when product has combinations on/off).
35ac6b6
to
1f94b21
Compare
1f94b21
to
103cdde
Compare
@Hlavtox we have to go forward, could you please remove your request approval? |
Hello @nicosomb , What are the steps to reproduce ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @nicosomb ,
Hummingbird is well installed.
Made an order with all types of products, order process is working as expected.
LGTM ✅
Thanks!
This PR allows to fix the issue I have here PrestaShop/PrestaShop#36198 (comment) to embed HB in PrestaShop core.