Skip to content
This repository has been archived by the owner on Dec 2, 2018. It is now read-only.

Fix issue https://github.com/ben-eb/perfectionist/issues/53 and add feature https://github.com/ben-eb/perfectionist/issues/21 #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Oct 6, 2016

Fix this issue: #53
Add feature by this issue: #21
mode name is vertical-align

Dmitriy Lodyanov added 2 commits October 6, 2016 22:45
Add feature by this issue: #21
mode name is vertical-align
Copy link
Owner

@ben-eb ben-eb left a comment

Choose a reason for hiding this comment

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

I like the changes in src/maxSelectorLength.js, no need for updating these. 😄

However, do you think that we could avoid the copy-paste via the src/applyVerticalAlign.js file? This makes it more difficult to maintain in the future. I'd like to suggest that we make this behaviour under a flag instead, and it might be nice to use the indent style guide for this.

What do you think of this proposal?

{
    indentStyle: 'otbs';
}

outputs (this is default):

h1 {
    color: blue;
}

And:

{
    indentStyle: 'allman'
}

outputs:

h1
{
    color: blue;
}

Also, could you please add some unit tests for these changes? That would be fantastic!

Thanks for your contribution!

@ghost
Copy link
Author

ghost commented Oct 10, 2016

Thank you for review. It will be time (counting this month), i will make needed changes. About new flag - indentStyle, and its values - i like it.
Tests - okey, i going add them.

@henryruhs
Copy link

@dmitriy-lodyanov Hello, how is the current status? I need the Allman style for my CSS :-)

@ghost
Copy link
Author

ghost commented Oct 31, 2016

@redaxmedia, sorry, busy in October, I expect to finish in November. Right now you can use fork with this commit, him work, but have bad option name choice and implementation.

@ben-eb
Copy link
Owner

ben-eb commented Feb 24, 2017

@dmitriy-lodyanov Hi, no rush; but do you have any time to do those changes? 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants