Skip to content

Feat/alert #47

Merged
merged 12 commits into from
Oct 10, 2018
Merged

Feat/alert #47

merged 12 commits into from
Oct 10, 2018

Conversation

roll314
Copy link
Contributor

@roll314 roll314 commented Sep 21, 2018

No description provided.

Peter Kornuishin added 5 commits July 4, 2018 19:18
# Conflicts:
#	src/lib-dev/navbar/styles.scss
#	src/lib/core/utils/utils.ts
#	src/lib/navbar/_navbar-base.scss
#	src/lib/navbar/navbar.component.ts
padding: 0;
}

> p,
Copy link
Member

Choose a reason for hiding this comment

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

Для чего?
Не используется далее, в примерах.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

убрал

<i mc-icon="mc-info_16"></i>
<div>
<header>Header</header>
Alert text Alert text Alert text Alert text Alert text Alert text Alert text Alert text Alert text
Copy link
Member

Choose a reason for hiding this comment

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

добавь пример с очень длинным текстом, больше 400 символов

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Добавил

styleUrls: ['./styles.scss'],
encapsulation: ViewEncapsulation.None
})
export class DemoComponent {}
Copy link
Member

Choose a reason for hiding this comment

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

добавь пример - show/hidden метод.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

добавил

src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
padding: 0;
margin: 0;
top: 16px;
right: 12px;
Copy link
Member

Choose a reason for hiding this comment

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

right - 16px, см макет
со всех сторон отступ 16px

Copy link
Contributor Author

Choose a reason for hiding this comment

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

У большого отступ 16 16 у маленького 16 12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

изменил согласно макетам

src/lib-dev/alert/template.html Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
Copy link
Contributor Author

@roll314 roll314 left a comment

Choose a reason for hiding this comment

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

fixed

src/lib/core/styles/_alerts.scss Outdated Show resolved Hide resolved
src/lib/core/styles/_alerts.scss Show resolved Hide resolved
@mikeozornin
Copy link
Contributor

Компоненты
Цвета иконок по дефолту должны совпадать с цветами алертов

Отступы алерта 14 16, а не 16 16. Это нужно, чтобы высота всего алерта в одну строку была кратна 8пк. Из-за текста высотой 20пк это сбивается и нужны эти 14 пк. А с учетом того, что бордер в CSS считается в границах блока, видимо нужно даже 13px 15px.

Текст не должен заезжать под крестик алерта, если крестик есть. Это не очень аккуратно смотрится.
http://d.mikeozornin.ru/A3BXm3
http://d.mikeozornin.ru/x5W2ZJ

Некликабельные иконки (http://d.mikeozornin.ru/elWMsJ) не должны хувериться. Сейчас они темнеют при хувере.

На крестик закрытия можно попасть табуляцией, но фокус не показывается.

http://d.mikeozornin.ru/vncuoM
Вот так по-моему неправильно. Нужно не напрямую обращаться к цветам, а обращаться к семантическим переменным из темы: info, success, warning и error. Если их нет, то пора завести, иначе будут большие рефакторинги потом.
@fost, глянь этот пункт.

Доки
http://d.mikeozornin.ru/qvH3YS
Заголовки секций не выровнены с алертами

http://d.mikeozornin.ru/tluklf
В мелком алерте кнопка смотрится очень тяжело, поэтому в примерах там псевдоссылка

Тайминги скрытия алертов очень длинные. Вот так лучше:
transition('false => true', animate('.5s')),
transition('true => false', animate('.2s'))

The button → Done или Close, и они тоже закрывают алерты.

Copy link
Contributor

@mikeozornin mikeozornin left a comment

Choose a reason for hiding this comment

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

@mikeozornin
Copy link
Contributor

mikeozornin commented Oct 6, 2018

Если что-то из того, что я написал в компоненты, к ним не относится, скажите, я мог перепутать.

@roll314
Copy link
Contributor Author

roll314 commented Oct 8, 2018

Компоненты
Цвета иконок по дефолту должны совпадать с цветами алертов
пофиксил

Отступы алерта 14 16, а не 16 16. Это нужно, чтобы высота всего алерта в одну строку была кратна 8пк. Из-за текста высотой 20пк это сбивается и нужны эти 14 пк. А с учетом того, что бордер в CSS считается в границах блока, видимо нужно даже 13px 15px.

сделал 14 16

Текст не должен заезжать под крестик алерта, если крестик есть. Это не очень аккуратно смотрится.
http://d.mikeozornin.ru/A3BXm3
http://d.mikeozornin.ru/x5W2ZJ

пофиксил

Некликабельные иконки (http://d.mikeozornin.ru/elWMsJ) не должны хувериться. Сейчас они темнеют при хувере.

пофиксил

На крестик закрытия можно попасть табуляцией, но фокус не показывается.

сделал через псевдо, неуверен, что мы должны мутить компонент для поддержки CanFocus. Есть еще вариант с введением нового компонента - прозрачной кнопки, которая не имеет никаких стилей, кроме поддержки фокуса и определенного "кнопочного" поведения. Или взять McIconButton и переопределить все в ней через !important (я думаю это породит много регрессии при правке кнопок)

http://d.mikeozornin.ru/vncuoM
Вот так по-моему неправильно. Нужно не напрямую обращаться к цветам, а обращаться к семантическим переменным из темы: info, success, warning и error. Если их нет, то пора завести, иначе будут большие рефакторинги потом.
@fost, глянь этот пункт.

сделал как константу в default_theme, предполагая, что это эти цвета не относятся к теме. Если относятся, то надо прокинуть их в миксин темы. @fost - как правильно?

Доки
http://d.mikeozornin.ru/qvH3YS
Заголовки секций не выровнены с алертами

сделал

http://d.mikeozornin.ru/tluklf
В мелком алерте кнопка смотрится очень тяжело, поэтому в примерах там псевдоссылка

сделал

Тайминги скрытия алертов очень длинные. Вот так лучше:
transition('false => true', animate('.5s')),
transition('true => false', animate('.2s'))

сделал

The button → Done или Close, и они тоже закрывают алерты.

сделал

@roll314
Copy link
Contributor Author

roll314 commented Oct 10, 2018

Обновил тему:

  • убрал warn - теперь он error
  • добавил все новый цвета в тему со значениями по-умолчанию

@roll314 roll314 closed this Oct 10, 2018
@roll314 roll314 reopened this Oct 10, 2018
@pimenovoleg pimenovoleg merged commit 5eea588 into positive-js:master Oct 10, 2018
@roll314 roll314 deleted the feat/alert branch October 25, 2018 07:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants