Skip to content

feat(popover): Pull request popover update #120

Merged

Conversation

LeonVay
Copy link
Contributor

@LeonVay LeonVay commented May 14, 2019

@fost, @lskramarov, @mikeozornin обновил пулл реквест, по комментариям прошёлся и поправил.
Позиционирование отрабатывает, в dev-примере все 12 положений поповера.
В процессе доработка по пробросу "размера" поповера по максимально разрешённой ширине (mikeozornin в курсе).

@LeonVay LeonVay force-pushed the feat(popover)-Pull_request_popover_update branch from 346285e to 16f5611 Compare May 14, 2019 08:27
@LeonVay
Copy link
Contributor Author

LeonVay commented May 14, 2019

Влил атрибут mcPopoverSize. Валидные значения 'small', 'normal', 'large'. Отрабатывают как пресеты на максимальную ширину. Если значение не из перечисленных - перевод в дефолтное normal. Есть идея выставить классы через host, но тогда в стилях будет :host[class] { max-width: $var }. Пробовать такое решение или пока что оставить как есть? @lskramarov

@lskramarov
Copy link
Contributor

Влил атрибут mcPopoverSize. Валидные значения 'small', 'normal', 'large'. Отрабатывают как пресеты на максимальную ширину. Если значение не из перечисленных - перевод в дефолтное normal. Есть идея выставить классы через host, но тогда в стилях будет :host[class] { max-width: $var }. Пробовать такое решение или пока что оставить как есть? @lskramarov

В стилях :host не будет, пробуй, так же можешь посмотреть модификаторы в других компонентах.

@mikeozornin
Copy link
Contributor

mikeozornin commented May 14, 2019

Компоненты

http://d.mikeozornin.ru/Ptpo3I
Меня очень смущает вот это выравнивание (по низу). Оно логично (нижние элементы вровень), но некрасиво. Предлагаю пока ничего не делать, но держать в голове (моей).

Если быстро кликать, то поповеры останутся:
http://d.mikeozornin.ru/0mnGbi

Отступы слева и справа в подвале чуть велики

Примеры

Давай добавим отступы между кнопками и уберем слово «position», оно везде одинаковое. А ещё можно выровнять кнопки так: http://d.mikeozornin.ru/jaoUmU (тут поповер всегда должен открываться внутрь, если я ничего не перепутал). Если так расположить сложно, то и ладно.

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.

.

@LeonVay
Copy link
Contributor Author

LeonVay commented May 15, 2019

@mikeozornin обновил, посмотри пожалуйста.
Проблему с "двойным" поповером решаю.

packages/mosaic/popover/popover.component.ts Outdated Show resolved Hide resolved
packages/mosaic/popover/popover.component.ts Outdated Show resolved Hide resolved
packages/mosaic/popover/popover.component.ts Outdated Show resolved Hide resolved
@LeonVay
Copy link
Contributor Author

LeonVay commented May 17, 2019

@mikeozornin , @lskramarov
добавил "анимацию" программную, чтобы не было коллизий с появлением двух экземпляров поповера за раз + геттер на классы поправил (теперь учитываются классы стилей хоста)
в CSS-ке добавил max-width для самого поповера и его контейнера с учётом обязательности (!important), чтобы не было "плавающей" ширины, если пользовательские классы хотят поменять ширину контейнера или компонента

@lskramarov
Copy link
Contributor

еще актуально

К тому же в компоненте:
@input('mcPopoverHeader') mcHeader: string | TemplateRef;
нет смысла объявлять @input('mcPopoverHeader'), если нигде нет возможности использовать этот интпут, а параметр mcHeader устанавливается через updateCompValue

@lskramarov
Copy link
Contributor

А разве после добавления анимации не нужно было убрать таймеры ?

@mikeozornin mikeozornin self-requested a review May 17, 2019 21:18
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.

По компоненту

  1. У меня как-то очень медленно вываливается поповер. У Лёни не видел такого на винде, а на маке ощутимая задержка. Вот видео:
    http://d.mikeozornin.ru/VBU6T6

  2. Если закрывать поповер кликом мимо, то следующий клик не открывает поповер. Два раза я открывал и закрывал поповер кликом в кнопку и два раза кликом мимо:
    http://d.mikeozornin.ru/VBU6T6
    На виндоузе такого опять же не видел.

↑ Такое поведение у меня во всех имеющихся браузерах: сафари, хром, фф. Если тебе нужно помочь подебажить скажи, ещё у нас есть общий мак в офисе, можно на нем попробовать.

  1. Видимо я в прошлый раз не заметил. Дятел (уголок) у поповера слишком большой, он на 3 пк больше по высоте и на 6 по ширине:
    http://d.mikeozornin.ru/lzFrka

  2. Позиционирование уголка относительно исходного элемента не всегда корректное. Этого сложно было понять из макетов, потому что макеты были без элементов, вызывающих поповер. В следующий раз будем добавлять в макеты элементы, с которыми компоненты взаимодействуют.
    Вот про позиционирование: http://d.mikeozornin.ru/ZJM3WI

Примеры

Сейчас кнопки расположены не так, это видимо из-за путающих названий left-right. У кнопки left поповер должен выпасть влево, но ему не хватит место поэтому фактически он выпадает вправо. Поэтому кнопки left нужно в примере справа, а right слева. Некоторые другие тоже перепутаны. Посмотри, пожалуйста, на http://d.mikeozornin.ru/jaoUmU. Я вроде там ничего не перепутал.

@LeonVay
Copy link
Contributor Author

LeonVay commented May 21, 2019

А разве после добавления анимации не нужно было убрать таймеры ?

Поправил

@LeonVay
Copy link
Contributor Author

LeonVay commented May 21, 2019

еще актуально

К тому же в компоненте:
@input('mcPopoverHeader') mcHeader: string | TemplateRef;
нет смысла объявлять @input('mcPopoverHeader'), если нигде нет возможности использовать этот интпут, а параметр mcHeader устанавливается через updateCompValue

Поправил

@LeonVay
Copy link
Contributor Author

LeonVay commented May 21, 2019

По компоненту

  1. У меня как-то очень медленно вываливается поповер. У Лёни не видел такого на винде, а на маке ощутимая задержка. Вот видео:
    http://d.mikeozornin.ru/VBU6T6
  2. Если закрывать поповер кликом мимо, то следующий клик не открывает поповер. Два раза я открывал и закрывал поповер кликом в кнопку и два раза кликом мимо:
    http://d.mikeozornin.ru/VBU6T6
    На виндоузе такого опять же не видел.

↑ Такое поведение у меня во всех имеющихся браузерах: сафари, хром, фф. Если тебе нужно помочь подебажить скажи, ещё у нас есть общий мак в офисе, можно на нем попробовать.

  1. Видимо я в прошлый раз не заметил. Дятел (уголок) у поповера слишком большой, он на 3 пк больше по высоте и на 6 по ширине:
    http://d.mikeozornin.ru/lzFrka
  2. Позиционирование уголка относительно исходного элемента не всегда корректное. Этого сложно было понять из макетов, потому что макеты были без элементов, вызывающих поповер. В следующий раз будем добавлять в макеты элементы, с которыми компоненты взаимодействуют.
    Вот про позиционирование: http://d.mikeozornin.ru/ZJM3WI

Примеры

Сейчас кнопки расположены не так, это видимо из-за путающих названий left-right. У кнопки left поповер должен выпасть влево, но ему не хватит место поэтому фактически он выпадает вправо. Поэтому кнопки left нужно в примере справа, а right слева. Некоторые другие тоже перепутаны. Посмотри, пожалуйста, на http://d.mikeozornin.ru/jaoUmU. Я вроде там ничего не перепутал.

Анимацию поменял, кнопки переставил, позиционирование для right-top\right-bottom\left-top\left-bottom поправил, уменьшил стрелку с 25px до 19px, клик исправил

@LeonVay LeonVay force-pushed the feat(popover)-Pull_request_popover_update branch from 88656a5 to 0d2eaba Compare May 24, 2019 11:17
@mikeozornin mikeozornin self-requested a review May 26, 2019 09:30
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.

Отлично, можно лить

@pimenovoleg pimenovoleg merged commit d2e885e into positive-js:master May 26, 2019
@LeonVay LeonVay deleted the feat(popover)-Pull_request_popover_update branch May 27, 2019 04:52
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