Skip to content

Conversation

hitesh-wagento
Copy link
Contributor

@hitesh-wagento hitesh-wagento commented Jul 11, 2018

Description

Some JS files are direct children of web rather than web/js. This does not follow instructions from the dev docs thus is confusing.

Fixed Issues (if relevant)

  1. JS files located outside the web/js directory #16302: JS files located outside the web/js directory

Expected result

  1. JS files to be inside web/js as per the dev docs.

Actual result

  1. JS files are direct children of web, this is inconsistent with the dev docs.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

Original PR

#16582

@magento-engcom-team magento-engcom-team added Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner Area: Frontend Component: multiple labels Jul 11, 2018
@magento-engcom-team
Copy link
Contributor

Hi @hitesh-wagento. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@hitesh-wagento
Copy link
Contributor Author

Hi @VladimirZaets

As per you comment in #16582 I have created new PR based on 2.3-develop branch.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jul 11, 2018

@VladimirZaets @hitesh-wagento what if we will just create map between old and new paths? In this case - this change will be backward compatible, so we can add them even to 2.2.x. Am I wrong?
Example:

var config = {
    map: {
        '*': {
            transparent: 'Magento_Payment/js/transparent',
            'Magento_Payment/transparent': 'Magento_Payment/js/transparent'
        }
    }
};

@larsroettig
Copy link
Member

@magento-engcom-team give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @larsroettig. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @larsroettig, here is your new Magento instance.
Admin access: https://pr-16708.engcom.dev.magento.com/admin
Login: admin Password: 123123q

@larsroettig
Copy link
Member

Manual Testing

  • Create new Produkt Backend
  • Checkout Produkt
  • Register Account
  • Captcha Passoword Forget
  • Captcha Register
  • Captcha Login
  • Order Grid
  • Forget Password

Manual Testing looks very good

@larsroettig larsroettig self-requested a review July 12, 2018 07:34
@hitesh-wagento
Copy link
Contributor Author

@larsroettig

Any update in this PR ?

Thanks

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please create PR to 2.2-develop branch first

  2. Please implement changes in all requirejs-config.js files that I described in #16708 (comment) (in 2.2-develop PR only)

  3. Please create separate commit for separate module in order to have ability apply these changes as described in https://support.magento.com/hc/en-us/articles/360005484154-Create-a-patch-for-a-Magento-2-Composer-installation-from-a-GitHub-commit

@hitesh-wagento
Copy link
Contributor Author

Hi @ihor-sviziev

Previously I have created PR based to 2.2-develop but @VladimirZaets told me created PR based on 2.3-develop. could you please suggest what can I do ?

#16582 (comment)


  1. Please create separate commit for separate module in order to have ability apply these changes as described in https://support.magento.com/hc/en-us/articles/360005484154-Create-a-patch-for-a-Magento-2-Composer-installation-from-a-GitHub-commit
    ==> Means do you need to 27 (Specific Module requirejs-config.js or rename files) files commit in one branch but different commit ID ?

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jul 18, 2018

Hi @hitesh-wagento ,

  1. These changes were requested to be done in 2.3-develop because they are not backward compatible. Now I added comment with info how we can make them backward compatible. So let's add backward compatible changes to 2.2-develop,
  2. I thought it will be few commits. Let's not split it to commit per module

If you have any questions - feel free to contact me in slack

@ihor-sviziev
Copy link
Contributor

Hi @hitesh-wagento,
We are having discussion about this specific case with @VladimirZaets, so please wait once we will finalize out general vision how it should be done. I will update you after discussion.

@VladimirZaets
Copy link
Contributor

Hi @hitesh-wagento. Please apply the fix provided by @ihor-sviziev to Magento 2.2 release line.
For Magento 2.3 release line your fix is ok and will be processed after PR to Magento 2.2 will be merged.

@hitesh-wagento
Copy link
Contributor Author

@VladimirZaets Thanks for reply.

@hitesh-wagento
Copy link
Contributor Author

Hi @VladimirZaets

I have completed all changes 2.2-develop related #16582

Thanks

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find changes that you did in: https://github.com/magento/magento2/pull/16582/files#diff-ce4bc7c513c965385f8696ebc9545f42
Everything else looks good

@hitesh-wagento
Copy link
Contributor Author

Hello @ihor-sviziev

I have checked again all module and added send friend module changes.

Thanks

@hitesh-wagento
Copy link
Contributor Author

@ihor-sviziev

Any update in this PR ?

Thanks

@sidolov sidolov changed the base branch from 2.3-develop to 2.2-develop September 5, 2018 13:12
@sidolov sidolov changed the base branch from 2.2-develop to 2.3-develop September 5, 2018 13:12
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 5, 2018

Hi @hitesh-wagento,
Looks like we have conflict during merging 2.3-develop and your PR. Probably it caused by moving some files.

Could squash all your changes into single commit and force push your changes or merge 2.3-develop branch?

@hitesh-wagento
Copy link
Contributor Author

Hello @ihor-sviziev

Could you please share with me steps how to merge all commits in one commit?

Thanks

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 7, 2018

Hi @hitesh-wagento could you just merge 2.3-develop branch from magento/magneto2 repository? It will be much easier

@@ -0,0 +1,22 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was removed in efdc2e3#diff-90e2539e902c667160d28b2cb3c56486
No need to create it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed and push again

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-2895 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @hitesh-wagento. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend Component: multiple Partner: Wagento Pull Request is created by partner Wagento partners-contribution Pull Request is created by Magento Partner Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants