Skip to content

LESS code clean up#18957

Merged
magento-engcom-team merged 14 commits intomagento:2.3-developfrom
Karlasa:Karlasa-less-clean-up
Nov 22, 2018
Merged

LESS code clean up#18957
magento-engcom-team merged 14 commits intomagento:2.3-developfrom
Karlasa:Karlasa-less-clean-up

Conversation

@Karlasa
Copy link
Copy Markdown
Contributor

@Karlasa Karlasa commented Oct 30, 2018

Description (*)

LESS code clean up to follow coding standards

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. ...
  2. ...

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)

@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @Karlasa. 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

Copy link
Copy Markdown
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

Removing .lib-css is not a good idea.

@DanielRuf
Copy link
Copy Markdown
Contributor

@DanielRuf
Copy link
Copy Markdown
Contributor

Also Update filename is not a clear commit message. Please rebase and reword them.

@Karlasa
Copy link
Copy Markdown
Contributor Author

Karlasa commented Oct 31, 2018

@DanielRuf Fixed some lib-css but for @indent/@font-weight it does not make sense having it as almost everywhere else it's called without it.

@DanielRuf
Copy link
Copy Markdown
Contributor

@DanielRuf Fixed some lib-css but for @indent/@font-weight it does not make sense having it as almost everywhere else it's called without it.

Why not? It is especially meant for variables and to merge rules and overwrite them without adding strikethrough rules on the browser side.

@Karlasa
Copy link
Copy Markdown
Contributor Author

Karlasa commented Oct 31, 2018

What do you mean by merge rules?

Why not?

if you have it without .lib-css() in 90% then i would never change it to false as i would end up cleaning up that 10% code but with 90% of incorrect code eg. padding: false/font-weight: false
So it would make sense to clean it up. Also not having padding in or font-weight in site is quite strange case. i get .lib-css() if it's more specific variable.
ex:
@minicart-padding: @indent__s;
.lib-css(padding, @minicart-padding);
would make sense for me as there are cases where you would like to disable it but not with that general variables

@VladimirZaets VladimirZaets self-assigned this Oct 31, 2018
@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @VladimirZaets, thank you for the review.
ENGCOM-3342 has been created to process this Pull Request

@Karlasa
Copy link
Copy Markdown
Contributor Author

Karlasa commented Oct 31, 2018

@VladimirZaets what is needed to be updated?

@VladimirZaets
Copy link
Copy Markdown
Contributor

Hi @Karlasa, I add "Need update" label because Daniel had some question according to fix. I looked the fix, and I think it is ok. Thanks for collaboration.

@magento-engcom-team magento-engcom-team merged commit 93af836 into magento:2.3-develop Nov 22, 2018
magento-engcom-team pushed a commit that referenced this pull request Nov 22, 2018
@magento-engcom-team
Copy link
Copy Markdown
Contributor

Hi @Karlasa. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants