Skip to content

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

Merged
mgechev merged 2 commits intomgechev:masterfrom
rafaelss95:feat-max-inline-declarations
Jun 7, 2018
Merged

feat(rule): add option in max-inline-declarations to limit animations…#569
mgechev merged 2 commits intomgechev:masterfrom
rafaelss95:feat-max-inline-declarations

Conversation

@rafaelss95
Copy link
Collaborator

@rafaelss95 rafaelss95 commented Apr 22, 2018

… 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

@rafaelss95 rafaelss95 Apr 22, 2018

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

@wKoza wKoza Apr 29, 2018

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

rafaelss95 commented Apr 24, 2018

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

rafaelss95 commented Jun 6, 2018

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

@mgechev mgechev mgechev approved these changes

@wKoza wKoza wKoza 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