-
Notifications
You must be signed in to change notification settings - Fork 188
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
#286: Remove percent sign for zero-values percentages when safe #361
Conversation
// Zero-value workaround | ||
(left.node.value === '0' && left.node.value === right.node.value) || | ||
// Because `CSSTree` returning `0%`, `0px` and etc, but must just `0` | ||
(translate(left.node) === translate(right.node)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Всё же мне кажется, что тут совсем не нужен CSSTree
, а проверка должна быть такой:
return (
(left.node.value === right.node.value) &&
(left.node.value === '0' || left.node.type === right.node.type)
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Думаю нужно в CSSTree реализовать функцию compare(node, node)
;)
Сравнивать через translate не очень круто в целом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compare(node, node)
всё же нет, ноды-то разные, просто ноль
исключение.
Привет! Спасибо за PR!
CSSTree возвращает то, что написали в CSS ;) А вот CSSO уже выкидывает ненужное. Собственно он откидывает единицы измерения у дименшенов с нулем, если эта единица измерения про длину – это относительно несложно. А вот процент откинуть сложнее, для этого нужно знать про что этот процент про длину, а он может быть про что угодно.
Если откинуть проценты у нулей, то изменения в |
@@ -0,0 +1 @@ | |||
.a,.b{margin:0%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Правильный ответ должен быть margin:0
@lahmatiy Всё так, но я считаю, CSSTree всё правильно сделал, оптимизировать и тем более что-то отбрасывать задача |
Я наверное плохо донес мысль
Имелось ввиду сделать аналог https://github.com/css/csso/blob/master/lib/replace/Dimension.js, но для Percent – тогда restructShorthand трогать не придется, он и так справится |
@lahmatiy аааа, только вот если добавить обработку ps так себе из меня помошник ;] |
поэтому нужно делать только для определенных свойств и если это не внутри функции
Я не просто так отправлял на, там это есть: https://github.com/css/csso/blob/master/lib/replace/Dimension.js#L39 ;) |
@lahmatiy блин, видно что-то я не так сделал и в прошлый раз у меня там |
lib/replace/Declaration.js
Outdated
@@ -0,0 +1,21 @@ | |||
var INDENT_PROPS = ['margin', 'padding'].reduce(function(ind, prop) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А как же другие свойства?
Список можно подобрать из словарей CSSTree (см в секции Used by элементы вида <'property-name'> – с кавычками это свойства, без – типы):
И лучше списком указывать свойства, и проще и наглядней чем генерация
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Странно, а почему в этом списке нет left, top
, но есть position
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аааа, кажись понял ;]
lib/replace/Dimension.js
Outdated
|
||
node.value = value; | ||
|
||
if (value === '0' && this.declaration !== null && this.atrulePrelude === null) { | ||
if (node.type === 'Percentage') { | ||
if (PERCENTAGE_LENGTH_DELC.hasOwnProperty(decl) || PERCENTAGE_LENGTH_FN.hasOwnProperty(fnName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lahmatiy оно?
Здорово, то что нужно! |
@lahmatiy да не вопрос, только даже если тебе понравиться, не мэржди это, слишком много мусора в комитах. |
Готово ;] |
lib/replace/Percentage.js
Outdated
@@ -0,0 +1,80 @@ | |||
var packNumber = require('./Number').pack; | |||
var PERCENTAGE_LENGTH_DELC = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PERCENTAGE_LENGTH_DELC
-> PERCENTAGE_LENGTH_PROPERTY
lib/replace/Percentage.js
Outdated
'transform-origin': true, | ||
'word-spacing': true | ||
}; | ||
var PERCENTAGE_LENGTH_FN = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PERCENTAGE_LENGTH_FN
-> PERCENTAGE_LENGTH_FUNCTION
lib/replace/Percentage.js
Outdated
|
||
node.value = value; | ||
|
||
if (PERCENTAGE_LENGTH_DELC.hasOwnProperty(decl) || PERCENTAGE_LENGTH_FN.hasOwnProperty(fnName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нужно поменять местами условия, сначала проверяем на ноль, а потом контекст. Проверка по словарю гораздо дороже, и нужна только для нулей, которые в целом не так часто встречаются.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Кста, я в своё время сравнивал hasOwnProperty
vs obj[name] !== void 0
и второй выиграл, а Егоров (оочень давно) говорил мне, что v8 строгое сравнение с void 0
по особому выполняет, поэтому и быстро.
lib/replace/Percentage.js
Outdated
node.value = value; | ||
|
||
if (PERCENTAGE_LENGTH_DELC.hasOwnProperty(decl) || PERCENTAGE_LENGTH_FN.hasOwnProperty(fnName)) { | ||
// Zero-value workaround |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Комментарий не нужен – это решение, а не workaround ;)
Было бы круто добавить пару тестов – на проверку свойств в которых делается подмена, и проверку функций. То есть:
Для этого есть |
Это да, это тоже одна из причин не брать ветку, а Обновил PR, добавил проверку на PS Слушай, а ведь можно выкинуть проверку на var PERCENTAGE_LENGTH_DELC = {...};
var isPercentageLengthDecl = generateIsMethodByMap(PERCENTAGE_LENGTH_DELC);
function generateByDict(dict) {
var body = Object.keys(dict).map(function(key) {
return 'case ' + JSON.stringify(key) + ': return true;';
}).join('\n');
return Function('prop', 'switch (prop){' + body + ';} return false;');
} м? я вот люблю кодогенерацию ;] |
Я думал о таком решении, да. Имхо лучше генерировать код статически, чем динамически. |
|
||
module.exports = function compressPercentage(node, item) { | ||
var value = packNumber(node.value, item); | ||
var decl = this.declaration !== null ? this.declaration.property : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decl -> property
Тесты добавишь? :) |
Конечно, только скажи куда или по какому принципу? |
Я тут обнаружил, что для Тест с перечнем всех свойств и функций, для которых убираем процент |
Чест говоря, это из серии |
Хотя если подумать, это даже логично :| |
Короче, суть:
isEqualValuePair
— отстой и решает проблему только частично, потому что дальше код всё равно проходит черезrequire('css-tree').translate
, который делает всё правильно и сохраняет единицы измерения, но... а данном случае они не нужны, единицы должен отбрасывать CSSO.Поэтому мне кажется, сравнивать через
isEqualValuePair
не такой уж и отстой, осталось только как-то добавить в4-restructShorthand.js
, что если свойство изMAIN_PROPERTY
и оно=== '0'
, то поменять (или заменить) тип ноды наNumber
.Ну или «Миша, всё не очень, давай по новой»...