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

Added tests for stylus cases #61

Merged
merged 1 commit into from
Jul 15, 2015
Merged

Added tests for stylus cases #61

merged 1 commit into from
Jul 15, 2015

Conversation

blond
Copy link
Member

@blond blond commented Mar 30, 2015

#36

9 passing
327 failing

To fix need to implement options:

@blond blond force-pushed the issue-36 branch 2 times, most recently from c6867d1 to e764f62 Compare March 31, 2015 09:01
@blond blond mentioned this pull request Jun 5, 2015
Open
@blond blond added ready and removed backlog labels Jun 15, 2015
@tavriaforever tavriaforever force-pushed the issue-36 branch 3 times, most recently from bda82d4 to fdf563f Compare July 4, 2015 14:26
@tavriaforever
Copy link
Contributor

@blond update PR

@tavriaforever tavriaforever added review and removed ready labels Jul 4, 2015
blond
blond reviewed Jul 5, 2015
View reviewed changes
package.json
@@ -51,12 +51,18 @@
"vow": "0.4.9"
},
"devDependencies": {
"bower": "^1.3.12",
Copy link
Member Author

Choose a reason for hiding this comment

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

Только точные зависимости.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@blond
Copy link
Member Author

blond commented Jul 5, 2015

Нужно отребейзить от master.

blond
blond reviewed Jul 5, 2015
View reviewed changes
techs/css-stylus.js
.defineOption('imports', 'include')
.defineOption('sourcemap', false)
.defineOption('autoprefixer', false)
.defineOption('compress')
Copy link
Member Author

Choose a reason for hiding this comment

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

У compress потерялся явный false

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@blond blond added ready and removed review labels Jul 5, 2015
sipayRT
sipayRT reviewed Jul 6, 2015
View reviewed changes
techs/css-stylus.js
@@ -7,13 +7,26 @@
* **Опции**
*
* * *String* **target** — Результирующий таргет. По умолчанию `?.css`.
* * *Object* **variables** — Дополнительные переменные окружения для `stylus`.
* * *Bool* **comments** — Добавляет разделяющие комментарии между стилями, содержащие относительный путь до блока.
Copy link

Choose a reason for hiding this comment

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

Bool - какое-то странное сокращение. Нужно везде поменять на Boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

blond
blond reviewed Jul 13, 2015
View reviewed changes
test/stylus-tech.test.js
'display:-ms-flexbox;' +
'display:flex;' +
'}',
browsersConfig = ['Explorer 10', 'Chrome > 30', 'Firefox > 21', 'Opera 12'];
Copy link
Member Author

Choose a reason for hiding this comment

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

Сложновато для теста.

Я бы выбрал какую-то версию IE, которая никогда не будет меняться, чтобы тест не протухал.

Copy link

Choose a reason for hiding this comment

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

Пока IE10 оторвет префикс, enb-stylus 10 раз перепишется :) Но я бы вообще оставил для теста только Хром

Copy link
Contributor

Choose a reason for hiding this comment

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

Давайте все таки оставим осла, потому новые версии хрома слишком быстро появляются и информация о старых версиях в браузерлисте будет исчезать.

@blond blond changed the title Added tests for stylus cases (WIP) Added tests for stylus cases Jul 13, 2015
@blond
Copy link
Member Author

blond commented Jul 13, 2015

@tavriaforever Написал вопросы и замечания.

sipayRT
sipayRT reviewed Jul 13, 2015
View reviewed changes
test/stylus-native-tests.js
}
)
.spread(function (source) {
var s = mockHelperFs.normalizeFile(source),
Copy link

Choose a reason for hiding this comment

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

я против таких переменных!

Copy link
Contributor

Choose a reason for hiding this comment

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

изменил

@sipayRT
Copy link

sipayRT commented Jul 13, 2015

@tavriaforever я всё

@tavriaforever
Copy link
Contributor

Обновил PR

blond
blond reviewed Jul 14, 2015
View reviewed changes
techs/stylus.js
*
* * *Boolean* **compress** – Минифицирует результат рендеринга stylus, по умолчанию `false`
*
* * *String* **prefix** – Добавляет префикс ко всем css классам, по умолчанию `false`
Copy link
Member Author

Choose a reason for hiding this comment

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

Так баг есть для css файлов или нет?

Copy link
Contributor

Choose a reason for hiding this comment

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

Нет бага. Если импорт css оставляем как есть, префикс там никак и не добавятся, так как импорт это лишь ссылка на исходный файл.

Copy link
Member Author

Choose a reason for hiding this comment

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

Значит есть баг :) Файлы блоков могут быть c css расширением.

Copy link
Contributor

Choose a reason for hiding this comment

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

Как можно добавить префиксы в импорте? )) Если стилус раскроет css – тогда он туда префиксы добавит.

Copy link
Member Author

Choose a reason for hiding this comment

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

С помощью postcss. Стайлус не раскрывает CSS-файлы, это делает postcss.

@blond
Copy link
Member Author

blond commented Jul 14, 2015

Тесты падают из-за сломаного кодстайла.

@blond
Copy link
Member Author

blond commented Jul 14, 2015

Написал ещё немного замечаний

tavriaforever added a commit that referenced this pull request Jul 15, 2015
@tavriaforever
Merge pull request #61 from enb-make/issue-36
081dce9 
Added tests for stylus cases
@tavriaforever tavriaforever merged commit 081dce9 into master Jul 15, 2015
@tavriaforever tavriaforever deleted the issue-36 branch July 15, 2015 08:31
@sipayRT
Copy link

sipayRT commented Jul 15, 2015

This was referenced Jul 15, 2015
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
proj:contrib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blond @tavriaforever @sipayRT @tadatuta @levonet