Skip to content
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

Fix confusing DontMergeAttributes modification #1963

Merged
merged 2 commits into from
Jul 21, 2019

Conversation

ReviakinAleksey
Copy link
Contributor

Mailing List thread:

Use DontMergeClass instead of confusing DontMergeAttributes modification.
Mark DontMergeAttributes as deprecated.
Added test for DontMergeClass and fixing docs for it css-selectors.adoc file.


"leave node class attribute for replacement without class" in {
// TODO: Since was agreed not to change current behaviour, create test for it (https://groups.google.com/forum/#!topic/liftweb/Nswcxoykspc)
Copy link
Member

Choose a reason for hiding this comment

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

Is this still a TODO or just an explanation of this test case?

Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

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

This looks good. One outstanding question on a todo comment, but otherwise I'm happy.

@ReviakinAleksey
Copy link
Contributor Author

Hi @farmdawgnation, this comment it only explaining why this weird test exists, and mention of fact that modification will not skip merging of class if replacement does not contains class attribute.

Do you want me to change comment content?

@farmdawgnation
Copy link
Member

Nope, that's fine.

@farmdawgnation farmdawgnation merged commit f47a58c into lift:master Jul 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants