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

[Fixes #108] expose use #111

Merged
merged 1 commit into from
Jan 15, 2016
Merged

[Fixes #108] expose use #111

merged 1 commit into from
Jan 15, 2016

Conversation

JiLiZART
Copy link
Contributor

No description provided.

@levonet levonet added the review label Oct 15, 2015
@JiLiZART JiLiZART changed the title expose use [Fixes #108] expose use Oct 15, 2015
blond
blond reviewed Dec 17, 2015
View reviewed changes
techs/stylus.js
renderer
.use(nib())
.import(nib.path + '/nib');
if (this._use && Array.isArray(this._use)) {
Copy link
Member

Choose a reason for hiding this comment

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

Это лишняя проверка, опция use по умолчанию массив.

@blond
Copy link
Member

blond commented Dec 17, 2015

Я написал замечания, поправь их и сделай rebase, чтобы я мог влить PR.

@JiLiZART
Copy link
Contributor Author

@blond Хорошо, поправлю

@JiLiZART
Copy link
Contributor Author

@blond готово

@blond
Copy link
Member

blond commented Dec 25, 2015

С ребейзом немного не получилось :) Необходимо, чтобы в результате твой PR состоял из одного коммита с только твоими измененими.

Для этого удали этот коммит https://github.com/JiLiZART/enb-stylus/commit/c9d3ce4f4f13c9450cc52aeb83f8249cfba6f548. Затем обнови, master до текущей актуальной версии:

git checkout master
git pull upstream master

В данном случае под upstream имею ввиду этот репозиторий. Добавить его можно так:

git remote add upstream git@github.com:enb-make/enb-stylus.git

Потом сделай в своей ветке ребейз от master:

git checkout exposeUse
git rebase master

Исправь конфликты и перетри изменения в своей ветке:

# исправь код файлов с конфликтами
git add conflict-file.js
git rebase --continue

git push fork exposeUse --force

В данном случае под fork имею ввиду твой репозиторий. Добавить его можно так:

git remote add fork git@github.com:JiLiZART/enb-stylus.git

blond
blond reviewed Dec 25, 2015
View reviewed changes
api.ru.md
@@ -175,6 +177,14 @@ Oбработка `url()` внутри файлов `.styl` и `.css`.

**Важно!** Опция работает только для файлов с расширением `.styl`.

### use

Тип: `Array`. По умолчанию: `[]`.
Copy link
Member

Choose a reason for hiding this comment

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

Давай поддержим возможность передать не только массив, но и 1 плагин.

@JiLiZART
Copy link
Contributor Author

@blond как то так?

blond
blond reviewed Dec 27, 2015
View reviewed changes
techs/stylus.js
* @param {String[]} [options.includes=[]] Adds additional path to resolve a path in @import
* and url().<br/>
* [Stylus: include]{@link http://bit.ly/1IpsoTh}
* <br/>
* Important: Available for Stylus only.
* @param {Function[]} [options.use=[nib(), rupture(), jeet()]] Allows to use plugins for Stylus.<br/>
Copy link
Member

Choose a reason for hiding this comment

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

После равно пишется значение по умолчанию, а не примеры использования.

@blond
Copy link
Member

blond commented Dec 27, 2015

как то так?

Да, всё так. Но возникло ещё пару замечаний, вроде финальных.

@blond
Copy link
Member

blond commented Jan 14, 2016

@JiLiZART, будешь добивать? ) Осталось-то совсем чуть-чуть.

@JiLiZART
Copy link
Contributor Author

Да
Пт, 15 янв. 2016 в 6:08, Andrew Abramov notifications@github.com:

@JiLiZART https://github.com/JiLiZART, будешь добивать? ) Осталось-то
совсем чуть-чуть.

—
Reply to this email directly or view it on GitHub
#111 (comment).

@JiLiZART
Copy link
Contributor Author

@blond поправил тесты и jsdoc, по поводу сравнения nib() с [nib()] надеюсь правильно тебя понял.

blond
blond reviewed Jan 15, 2016
View reviewed changes
test/stylus-tech.test.js
@@ -300,6 +300,44 @@ describe('stylus-tech', function () {
});
});

describe('use', function () {
var nibPlugin = require('nib'),
vow = require('vow');
Copy link
Member

Choose a reason for hiding this comment

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

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

Ну и вообще, хороший тон все модули подключать не в рантайме, если в этом нет явной необходимости.

@blond
Copy link
Member

blond commented Jan 15, 2016

по поводу сравнения nib() с [nib()] надеюсь правильно тебя понял.

Да, всё правильно понял.

@blond
Copy link
Member

blond commented Jan 15, 2016

Написал небольшое замечание в тестах.

@JiLiZART
Copy link
Contributor Author

Поправил.

@blond
Copy link
Member

blond commented Jan 15, 2016

LGTM

blond added a commit that referenced this pull request Jan 15, 2016
@blond
Merge pull request #111 from JiLiZART/exposeUse
2f61cbc 
[Fixes #108] expose use
@blond blond merged commit 2f61cbc into enb:master Jan 15, 2016
@blond blond removed the review label Jan 15, 2016
@blond
Copy link
Member

blond commented Jan 15, 2016

@JiLiZART, спасибо за терпение! ;)

@blond blond added this to the 2.3.0 milestone Jan 15, 2016
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
None yet
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@JiLiZART @blond @levonet