Skip to content

[WIP] feat(tooltip): Tooltip component #33

Merged
merged 1 commit into from
Sep 21, 2018
Merged

[WIP] feat(tooltip): Tooltip component #33

merged 1 commit into from
Sep 21, 2018

Conversation

LeonVay
Copy link
Contributor

@LeonVay LeonVay commented Aug 9, 2018

Тултип WIP.
Запускать "server-dev:tooltip": "npm run server-dev -- --env.component tooltip"
Первая кнопка показывает тултип по фокусу, две последнии по клику.

@pimenovoleg pimenovoleg changed the title feat(tooltip): Tooltip component (WIP) [WIP] feat(tooltip): Tooltip component Aug 9, 2018
@pimenovoleg
Copy link
Member

Congratulations on your first PR!!!

@mikeozornin
Copy link
Contributor

Тултипы не провязаны с цветами из темы и типографикой.
Цвет должен быть серый из палитры, текст по тултипу — контраст к серому. Параметры шрифта должны совпадать со стилями из типографики, конкретно — миксин caption.

Не уверен, но кажется что пропадает тултип не сразу. Вот как надо:

Тултип появляется при наведении курсора на элемент, с задержкой в 400 миллисекунд. Это нужно чтобы при движении курсора по странице не было моргания тултипов.
Исчезает тултип мгновенно, как только курсор «уйдет» с элемента. Тултип исчезает и в том случае, если после его появления прокрутить страницу.

Примеры

При выглядит неаккуратно. Сейчас кнопки дефолтного стиля и перемешаны по логике.
Если сохранять кнопки, то лучше сделать как-то так:
image

Тексты лучше писать более-менее реальные, чем реальней тем лучше. Вот примеры, можно расставить произвольно по тултипам:

Удалить файл
Подсказка может занимать две и более строк
PDQL-запрос фильтра содержит ошибки
PDQL-запрос фильтра содержит ошибки. Uncaught SyntaxError: Unexpected string

Если можно сделать в тексте переносы строк, то было бы круто это показать и проверить в примере. Например вот один текст для тултипа из двух строк:

В группе «Серверы: Windows» PDQL-запрос содержит ошибки
Группы «Серверы: Windows (old)» был удалены

@pimenovoleg pimenovoleg requested a review from lskramarov August 13, 2018 18:16
@mikeozornin
Copy link
Contributor

Про сами тултипы
Если кликать по элементам с клавиатуры можно вызвать 4 тултипа сразу.
image
Пока открыты эти тултипы не показываются другие, привязанные на хувер: Скринкаст: http://d.mikeozornin.ru/w0RnZJ

В тултипе типографика своя, а не взята из миксинов типографики. Посмотри как в навбаре, например, там вроде правильно используется. Только в тултипах не title, а caption.
image

Про доки
Мы можем сделать в примерах тоже мозаичные стили? Сейчас там отсутствие стилей для кнопок и текста, из-за этого кажется что какие-то стили сломались.
Подписи — Title или body, кнопки дефолтные.
Вот как сейчас:
image

Давай ещё кнопки соберем ближе к центру? Даже при достаточно широком окне не хватает места, чтобы в левой колонке left'у хватило места, он переносится наверх. Я сначала не понял, что происходит.
Скриншот большой, поэтому ссылкой: http://screenshots.ptsecurity.com/mozornin-2018-08-14_16-24-43.png

@mikeozornin
Copy link
Contributor

Про сами тултипы
Если кликать по элементам с клавиатуры можно вызвать пять тултипов сразу.
image

Пока открыты эти тултипы не показываются другие, привязанные на хувер: Скринкаст: http://d.mikeozornin.ru/w0RnZJ

Вот здесь в типографике должен быть не small-text, а caption.
image
Уточни у @fost или @lskramarov как правильно брать свойства. Все пять отдельно, или как-то по-другому.

Примеры
На кнопках дефолтных жирноват текст, но я это уже чинил.

@mikeozornin
Copy link
Contributor

А, забыл написать. Новое позиционирование (когда уголок у right и left сверху, а не по середине) — отлично.

@LeonVay
Copy link
Contributor Author

LeonVay commented Aug 20, 2018

@mikeozornin выставил caption вместо small-text. Клик-ивент убрал, теперь Enter не будет работать. Обновил немного пример - повесил тултип на элементы списка, так же добавил пример со свойством mcTooltipDisabled.

@mikeozornin
Copy link
Contributor

На вид все хорошо.

@lskramarov
Copy link
Contributor

На вики должны быть доки для компонента

@lskramarov
Copy link
Contributor

lskramarov commented Aug 22, 2018

Опиши в кратце, общую концепцию работы тултипа, например:
Директива McTooltipDirective после инициализации делает то то...
по эвенту hover происходит то то...

То что я сейчас вижу по коду меня не очень радует, но возможно я что то не понимаю...

console.log(this.availablePositions);

Object.keys(this.availablePositions).some((key) => {
if (JSON.stringify($event.connectionPair) === JSON.stringify(this.availablePositions[ key ])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

я думаю найдется не один способ сделать это без JSON.stringify


$shadow-color: rgba(0, 0, 0, 0.2);
$border-radius-base: 3px;
$box-shadow-base: 0 2px 4px 0 $shadow-color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Все что касается цвета и типографики должно быть в теме т.е. нужно было перенести только эту строчку. Посмотри как сделано в других компонентах.

@lskramarov
Copy link
Contributor

Нужно исправить ошибки линтера:
overlay-ref.ts содержит 56 ошибок
tooltip.component.ts содержит 37 ошибок

@lskramarov
Copy link
Contributor

не нашел команды запуска дев примера.

@lskramarov
Copy link
Contributor

lskramarov commented Sep 12, 2018

в дев примере при фокусировке на первой кнопке тултип не появляется.
feat(tooltip): Removing tooltip element from DOM unless it is triggered to show - в этом коммите почему то тултип был удален.

@lskramarov
Copy link
Contributor

src\lib-dev\tooltip\module.ts - нужно почистить, содержит неиспользуемый код, видимо копипаст...

@lskramarov
Copy link
Contributor

Насколько Я вижу, теперь тултип удаляется из DOM сразу же после добавления.
Причем, в дальнейшем все корректно отрабатывает и тултип добавляется в оверлей.

Считаю, что проблема не решена, нужно разбираться и убрать такое поведение.

@lskramarov
Copy link
Contributor

Ошибки в тестах нужно поправить

@lskramarov
Copy link
Contributor

вот такое в примере

image

@lskramarov
Copy link
Contributor

lskramarov commented Sep 17, 2018

В документации mcMouseEnterDelay = 400, но на самом деле вот так:
2018-09-17_10-50-49

@lskramarov
Copy link
Contributor

Еще смущает ESC для тултипа, но если так было задумано то...

В остальном все отлично, после исправления:
тестов, положения в примере и delay можно будет влить.

ngOnInit(): void {
const element: any = this.elementRef.nativeElement;
// tslint:disable-next-line
ngOnInit(): void {}
Copy link
Contributor

Choose a reason for hiding this comment

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

С какой целью добавлен пустой метод ?


// tslint:disable-next-line
ngAfterViewInit(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

А какой смысл делать какие либо действия в ngAfterViewInit, если они никак не связаны с view ?


_afterVisibilityAnimation(e: AnimationEvent): void {
if (e.toState === 'false' && !this.mcVisible) {
this.hideTId = setTimeout(() => {
Copy link
Contributor

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.

Не должен по идее. Возвращаемое значение у setTimeout -> number. Пробовал invalidate сделать, у меня ошибки не появляются.

this._markForCheck();
if (!this.isContentEmpty()) {
this._closeOnInteraction = true;
this.showTId = setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

линтер ругается на тип

feat(tooltip): Fixing tests

feat(tooltip): MR comments partly fixed

feat(tooltip): Updating implementation

feat(tooltip): Tooltip typography

feat(tooltip): Some positioning updates

feat(tooltip): Mr fixes: click event removed, example updated

feat(tooltip): Fixing unit tests

feat(tooltip): Fix positioning change on scroll strategy

feat(tooltip): Removing console log and unused variables from styles

feat(tooltip): Update after rebase

feat(tooltip): Removing tooltip element from DOM unless it is triggered to show

feat(tooltip): Moving styles

feat(tooltip): Merge request changes

feat(tooltip): Minor changes connected to overlay

feat(tooltip): Updating positioning logic

feat(tooltip): Linter errors fix

feat(tooltip): Destroy tooltip component after hide

feat(tooltip): Fixing issues with show\hide delay and positioning

feat(tooltip): Updating tests

feat(tooltip): Renaming error throw method for positioning
@pimenovoleg
Copy link
Member

@lskramarov approve it?

@pimenovoleg pimenovoleg merged commit 9bfc4f1 into positive-js:master Sep 21, 2018
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