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.

Sign up for GitHub

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

Jump to bottom

feat(rule): add option in max-inline-declarations to limit animations… #569

Merged
merged 2 commits into from
Jun 7, 2018
Merged

feat(rule): add option in max-inline-declarations to limit animations… #569

merged 2 commits into from
Jun 7, 2018

Conversation

rafaelss95
Copy link
Collaborator

… lines

Closes #568.

rafaelss95
rafaelss95 commented Apr 22, 2018
View reviewed changes
src/maxInlineDeclarationsRule.ts Outdated
return generateFailure('template', limit);
};

export const animationsLinesDefaultLimit = 7;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about this default limit...

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me too little. Animation is quickly verbose. 15 lines, it is more common for a simple animation.

@wKoza
Copy link
Collaborator

wKoza commented Apr 23, 2018

This PR is confused because you are probably used Prettier. Two possibilities:

  • We can wait for your Prettier PR
  • You can revert changes in relation to 'Prettier'

@rafaelss95
Copy link
Collaborator Author

Yes, since I've installed Prettier in my extensions, it always format when I save a file. Anyway, what is confusing for you?

@mgechev
Copy link
Owner

mgechev commented Apr 24, 2018

@rafaelss95 I guess the big diff. Let's merge your PR which introduces prettier and after that merge this one.

@mgechev
Copy link
Owner

mgechev commented Apr 25, 2018

@rafaelss95 would you give examples what the role of transformAnimations would be?

@mgechev
Copy link
Owner

mgechev commented Apr 27, 2018

@rafaelss95 I can see some value in the rule. I'm not yet sure we should introduce it though.

@rafaelss95
Copy link
Collaborator Author

rafaelss95 commented Apr 28, 2018 •
edited
Loading

@mgechev Sorry for the delayed response. To read animations parameter, I've followed what you did for styles and template. The intent of this PR is just to add another option for the existent rule (max-inline-declarations): limit animation lines. We could increase the default lines limit for animation.

@mgechev
Copy link
Owner

mgechev commented May 2, 2018

Let's see if we'll collect enough votes for this feature before merging.

@rafaelss95
Copy link
Collaborator Author

bump @mgechev

@mgechev mgechev merged commit 25f3e16 into mgechev:master Jun 7, 2018
mgechev added a commit that referenced this pull request Jun 9, 2018
@mgechev
Merge branch 'master' of github.com:mgechev/codelyzer
615d57b 
* 'master' of github.com:mgechev/codelyzer:
  feat: externalizing template, css visitor abstractions and ngwalker (#658)
  feat(rule): add option in max-inline-declarations to limit animations lines (#569)
@rafaelss95 rafaelss95 deleted the feat-max-inline-declarations branch June 12, 2018 23:40
@rafaelss95 rafaelss95 mentioned this pull request Jun 24, 2018
Closed
@mgechev mgechev added this to the 4.4.0 - Ken Thompson milestone Jun 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wKoza wKoza wKoza approved these changes

@mgechev mgechev mgechev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.4.0 - Ken Thompson
Development

Successfully merging this pull request may close these issues.
3 participants
@rafaelss95 @wKoza @mgechev