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

page: Fix x-ua-compatible support #795

Merged
merged 1 commit into from
Mar 16, 2015
Merged

page: Fix x-ua-compatible support #795

merged 1 commit into from
Mar 16, 2015

Conversation

tadatuta
Copy link
Member

closes #794

// cc @mishanga

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c69ba47 on issues/794@v2 into 4486b82 on v2.

def().match(function() {
return typeof this.ctx['x-ua-compatible'] !== 'undefined';
})(function() {
applyNext({ xUaCompatible : this.ctx['x-ua-compatible'] });
Copy link
Member

Choose a reason for hiding this comment

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

Весь этот сатанизм нельзя заменить конструкцией var xUaCompatible = this.ctx['x-ua-compatible']?
Напомню, что наши разнообразные эксперименты с шаблонизаторами однозначно показали: надо писать шаблоны проще. Матчинг по дефолтной моде и кастомные подпредикаты сильно снижают и производительность, и читабельность шаблонов.

Copy link
Member

Choose a reason for hiding this comment

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

Тогда нельзя будет переопределять в шаблонах проекта, не?

@mishanga
Copy link
Member

@dfilatov мы, кажется, специально в свое время сокращали объём кода шаблонов блока page и упрощали API.

@tadatuta
Copy link
Member Author

@mishanga

Весь этот сатанизм нельзя заменить конструкцией var xUaCompatible = this.ctx['x-ua-compatible']?

Дело в том, что API page предполагает, что у блока есть массив ctx.head, где лежит все, что попадет в <head>. И есть поведение по умолчанию, когда x-ua-compatible не определено, то оно выставляется в IE=edge. А воспользоваться значением ctx x-ua-compatible нужно из элемента. Я не придумал, как это сделать красивее. Этот же ответ и про tParam.

Может я что-то упускаю, но предыдущая реализация просто никак не работала.

@mishanga
Copy link
Member

Ну так мы сейчас прямо в шаблоне генерим BEMJSON для элемента head, можно сразу там и пробросить поле x-ua-compatible в элемент.
https://github.com/bem/bem-core/blob/v2/common.blocks/page/page.bh.js#L18

@tadatuta
Copy link
Member Author

@mishanga вообще можно, но тогда придется класть код в common, хотя нужен он только для десктопа.

@tadatuta
Copy link
Member Author

@mishanga @narqo какой вариант исправления все-таки предпочтительнее: правильно разносим по уровням, но код сложнее или все в common, но код проще?

@Guria
Copy link
Contributor

Guria commented Jan 25, 2015

В BH на уровне проекта у меня работает такой вариант:

  bh.match('page__head', function(ctx) {
    ctx.json()['x-ua-compatible'] = 'IE=10';
  });

@Guria
Copy link
Contributor

Guria commented Jan 25, 2015

Кажется понял. Речь то о прокидывании значения из BEMJSON блока page.
Тогда мой комментарий не в тему.

@veged veged added v2 and removed v2 labels Jan 29, 2015
@qfox qfox added the v2 label Jan 30, 2015
@narqo
Copy link
Member

narqo commented Mar 5, 2015

@tadatuta ping!

@tadatuta
Copy link
Member Author

tadatuta commented Mar 5, 2015

@narqo я тут в #795 (comment) как раз интересуюсь, какой вариант решения вам больше нравится

@tadatuta tadatuta force-pushed the issues/794@v2 branch 3 times, most recently from a1de326 to f15eec5 Compare March 14, 2015 14:42
@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.57% when pulling f15eec5 on issues/794@v2 into 4090469 on v2.

@tadatuta
Copy link
Member Author

@narqo @mishanga 🆙

({
block : 'page',
title : 'Remove x-ua-compatible',
'x-ua-compatible' : false
Copy link
Member

Choose a reason for hiding this comment

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

What if we simplify it a little and use uaCompatible as a field name?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.57% when pulling e132782 on issues/794@v2 into 09d4d7f on v2.

@tadatuta
Copy link
Member Author

@narqo done

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.57% when pulling 6de37c0 on issues/794@v2 into 09d4d7f on v2.

narqo pushed a commit that referenced this pull request Mar 16, 2015
page: Fix x-ua-compatible support
@narqo narqo merged commit 6f18962 into v2 Mar 16, 2015
@narqo narqo deleted the issues/794@v2 branch March 16, 2015 21:16
@tadatuta tadatuta self-assigned this Jun 13, 2015
tadatuta added a commit that referenced this pull request Jun 13, 2015
closes #795
backport from v2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants