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

Control grid option via comment #1138

Merged
merged 2 commits into from Oct 12, 2018
Merged

Control grid option via comment #1138

merged 2 commits into from Oct 12, 2018

Conversation

fanich37
Copy link
Contributor

  • Add gridStatus method to Processor

  • Add test case

  • Update README.md

* Add gridStatus method to Processor

* Add test case

* Update README.md
@ai
Copy link
Member

ai commented Oct 11, 2018

@fanich37 awesome! One last thing. You need to improve test. Right now tests doesn’t execute every line of code.

Run:

npx jest --coverage

It will show what lines was not executed

@ai
Copy link
Member

ai commented Oct 11, 2018

Seems like yoou need to test result.warn call on double autoprefixer grid comments.

@Dan503
Copy link
Contributor

Dan503 commented Oct 11, 2018

Can you also add an /* autoprefixer grid: off */ command?

It should work both ways, you can turn the setting on if you have grid disabled by default or off if you have it enabled by default.

BTW, I ❤ the idea 😊

@Dan503
Copy link
Contributor

Dan503 commented Oct 11, 2018

Oh, you did, you just didn't add it to the readme.

Can you add the off command to the readme?

@fanich37
Copy link
Contributor Author

fanich37 commented Oct 12, 2018

Seems like yoou need to test result.warn call on double autoprefixer grid comments.

@ai Sure, I'll add them 👌

But, to be honest, I don't like the behaviour of gridStatus method. What I mean - during the processor execution gridStatus fires several times and it's ok if the rule itself has two or more control comments. But if these control comments are located in the root of the doc - then the console gets polluted with identical warnings. I'll try to figure out the solution for it.

Update: I missed one thing. Now it works as expected.

@fanich37
Copy link
Contributor Author

Oh, you did, you just didn't add it to the readme.

Can you add the off command to the readme?

@Dan503 Sure, I'll add it in the next commit 👌

* Remove unused code
* Update test
* Update README.md

if (node.parent) {
let p = node.prev()
if (p && p.type === 'comment' && IGNORE_NEXT.test(p.text)) {
Copy link
Member

Choose a reason for hiding this comment

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

А нам нужно это? Мы же всегда вызываем disabled() до запуска gridStatus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Блин, выходит, что нет...

}

let value = null
if (node.nodes) {
Copy link
Member

Choose a reason for hiding this comment

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

node — это свойство, к которому мы ходим добавить префикс? Зачем тогда мы смотри его детей? Надо смотреть node.parent.nodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Подозревал, что не до конца разобрался... Спасибо за ревью, Андрей 🙏

@ai
Copy link
Member

ai commented Oct 12, 2018

But, to be honest, I don't like the behaviour of gridStatus method. What I mean - during the processor execution gridStatus fires several times

It creates _autoprefixerGridStatus cache, so multiple calls will be fast

@fanich37
Copy link
Contributor Author

But, to be honest, I don't like the behaviour of gridStatus method. What I mean - during the processor execution gridStatus fires several times

It creates _autoprefixerGridStatus cache, so multiple calls will be fast

Yes, I added it in my last commit right after I realised what it does 😃

@fanich37 fanich37 deleted the grid-status branch October 15, 2018 05:25
@Dan503
Copy link
Contributor

Dan503 commented Oct 15, 2018

@fanich37 I am writing an article about all the awesome new stuff that is in version 9.2.0.

Are you happy for me to call you by the name "Andrey Alexandrov" in the article?

Where would you like your name to link to? At the moment I'm linking to your GitHub account. Is there somewhere else you would prefer I link to like a personal website, your linked in profile, or your twitter profile?

@fanich37
Copy link
Contributor Author

@fanich37 I am writing an article about all the awesome new stuff that is in version 9.2.0.

Are you happy for me to call you by the name "Andrey Alexandrov" in the article?

Where would you like your name to link to? At the moment I'm linking to your GitHub account. Is there somewhere else you would prefer I link to like a personal website, your linked in profile, or your twitter profile?

Wow! That's awesome! Of course you can call me by name. And my GitHub account is the only social thing I'm actively using😃 Thank you, Daniel🙏

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

Successfully merging this pull request may close these issues.

None yet

3 participants