Skip to content

feat(vert-navbar): initial implementation #102

Merged
merged 45 commits into from
May 7, 2019

Conversation

rt-gavrilov
Copy link
Contributor

No description provided.

@welcome
Copy link

welcome bot commented Apr 1, 2019

Congratulations on your first PR!

@mikeozornin
Copy link
Contributor

Артем!

Не пугайся размера комментария, тут много мелочей, часто их даже больше, чем здесь комментарии.

Сам компонент (как я смог понять)

  1. Иконки лучше не менять размером, для примера это не так важно, но важно в продуктах. Если нужного размера нет, нужно попросить дизайнера сделать именно нужный размер. Это важно, от этого зависит будут ли иконки выглядеть целостно, или чужеродно. Здесь нужны минимум:
    гамбургер
    крестик
    стрелка >
    колокольчик
    в размере 32 пк. Я их сделаю в публичный mosaic-icons.

  2. При сворачивании логотип сразу пропадает сразу и название продукта прыгает влево. По-моему, достаточно делать как в остальных пунктах, когда правый край просто закроет собой название продукта, а потом и логотип, чтобы в фоне ничего не скакало.

  3. Гамбургер не того цвета, а в светлой теме его даже плохо видно. Он должен быть такого же цвета, как остальные иконки. Если остальные пункты могут быть частью примера, то гамбургер — это часть компонента.

  4. У пункта с гамбургером-крестиком нет хувера и фокуса, хотя он не должен отличаться от остальных пунктов по хуверу и фокусу.
    Если название продукта кликабельно, то у него должен быть такой же хувер и такой же фокус. Если просто текст, то не должно быть ни того, ни другого.

  5. У названия продуктах сохранился стандартный аутлайн
    http://screenshots.ptsecurity.com/mozornin-2019-04-02_19-48-38.png

  6. Сейчас у выбранного элемента сохраняется фокус, даже если мы не использовали клавиатуру, и фокусных рамок может быть несколько. Фокус может быть только один, и он должен появляться только если использовали клавиатуру. Там есть какой-то хак, который делали в кнопках. Можно у Лёни спросить, если не найдешь.
    http://screenshots.ptsecurity.com/mozornin-2019-04-02_19-55-20.png

  7. Названия пунктов не надо переносить, если они в одну строку, значит в одну. Если длинные, пусть обрежутся
    http://screenshots.ptsecurity.com/mozornin-2019-04-02_19-56-34.png

  8. Последний пункт с профилем раскрывает подменю только в развернутом состоянии, должно не зависеть от свернутости-развернутости.

  9. Отступ от бейджа сверху и справа сейчас по 2 пк, а надо подальше — по 4 пк.
    http://screenshots.ptsecurity.com/mozornin-2019-04-02_20-12-10.png

  10. Не знаю, часть ли это компонента, но active-состояние должно быть без хувера. Фокус пускай будет как у всех, но cursor:pointer не нужен. Если внутри active-пункта есть вложенные, то хувер нужен. Идея в том, чтобы повторно не давать выбирать выбранный пункт, чтобы он даже не был кликабельным. Если это логика должна быть в продукте — ок, пусть будет там.

  11. Давай немного ускорим сворачивание?
    300 → 200
    200 → 150

Общее с горизонтальным навбаром

  1. Цвет иконок и текста должен быть не color-100, а контрастный к фону color: mc-contrast($palette, 700); (700 — это BG навбара). Видимо в обычном навбаре я это не заметил, а тут иконки крупнее и заметно.

  2. Рамка фокуса в 1 пк толщиной, а надо в 2 пк. Посмотрел, в навбаре так же, можно сразу и там поправить.

Про пример, а не про сам компонент

  1. В логотипе не нужна тень.

  2. Давай сделаем у колокольчика бейд синего цвета, а не ворнинговый. Ворнинговый кажется должен быть реже.

  3. Аватарка: белый текст на желтом не видно, давай фон сделаем как в макете чуть более светлый primary-цвет? Фон круг должен всего лишь чуть выделяться, а не бросаться в глаза. Плюс yellow-цвет не из палитры совсем. Писать поверх круг нужно контрастным к цвету круга, он будет такой же, как контрастный к цвету навбара.

  4. Custom item совсем какой-то custom, давай ему хотя бы сделаем шрифт нормальный? Сделать body-стиль цвета контрастного к навбару. Сейчас как будет пункт сломался.

  5. У меня пример показывается так, что не занимает все по высоте, и можно прокрутить вниз. У людей может сложиться впечатление, что это баг компонента. Давай сделаем так, чтобы навбар растягивался на 100% по высоте? Если нужно, можно убрать редкие пункты (все виды дизейбла и прогресс)

@lskramarov
Copy link
Contributor

навбар не должен реализовывать этот функционал выпадающего меню:
image

на макетах этого не было.

Нужно убрать, это значительно упростит компонент.

@mikeozornin @fost если нам нужен компонент меню, то его нужно запланировать.

@mikeozornin
Copy link
Contributor

А по-моему, вложенное меню является частью навбара.

@lskramarov
Copy link
Contributor

Это не все замечания, еще буду смотреть.

@lskramarov
Copy link
Contributor

А по-моему, вложенное меню является частью навбара.

нет, это реализует отдельный компонент:
https://material.angular.io/components/menu/examples

@mikeozornin
Copy link
Contributor

Осталось буквально два момента.

http://screenshots.ptsecurity.com/mozornin-2019-04-09_13-56-36.png
На все пункты кроме гамбургера и названия продукта нельзя попасть табуляцией. Нужно попадать на все, кроме дизебленных.

http://screenshots.ptsecurity.com/mozornin-2019-04-09_14-01-00.png
Если в свернутом состоянии нажать на табуляцию, то таб попадет на название продукта и будет плохо. В свернутом можно выкидывать название продукта из табулируемых элементов. Ок, если понадобится разворачивать меню, чтобы кликнуть по продукту.

И потом можно делать выпадающее вложенное меню.

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.

.


const [overlayY, overlayFallbackY]: VerticalConnectionPos[] =
this.dropdown.yPosition === 'above' ? ['bottom', 'top'] : ['top', 'bottom'];
let [originX, originFallbackX, overlayX, overlayFallbackX]: HorizontalConnectionPos[] =
Copy link
Member

Choose a reason for hiding this comment

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

были изначально const - причина изменения на let ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

изменяется ниже по коду

['bottom', 'top', 'bottom', 'top'] :
['top', 'bottom', 'top', 'bottom'];

let offsetY = 0;
Copy link
Member

Choose a reason for hiding this comment

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

было изначально const - причина изменения на let ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

изменяется ниже

@mikeozornin mikeozornin self-requested a review April 15, 2019 23:37
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.

С навбаром все хорошо.

В выпадающим меню есть некоторые моменты.

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

Возможно это дальше про пример.
Давай не класть в один уровень элементы с иконкой 32 пк, 16 пк и без иконки, выглядит не очень. Если нужно, можно разбить на три корневых уровня (одинаковые, по 32 пк), а в каждом подменю использовать элементы своей верстки (без иконок, 16 пк, 32 пк).

Между иконкой 16 пк и текстом мало отступа.
Между иконкой 32 пк и текстом мало отступа (в навбаре ок)

src/lib/dropdown/dropdown-item.ts Outdated Show resolved Hide resolved
src/lib/dropdown/dropdown-trigger.ts Outdated Show resolved Hide resolved
src/lib/dropdown/dropdown-trigger.ts Outdated Show resolved Hide resolved
src/lib/dropdown/dropdown-trigger.ts Outdated Show resolved Hide resolved
src/lib/dropdown/dropdown.component.ts Show resolved Hide resolved
src/lib/vertical-navbar/vertical-navbar.component.html Outdated Show resolved Hide resolved
src/lib/vertical-navbar/vertical-navbar.component.html Outdated Show resolved Hide resolved
src/lib/vertical-navbar/vertical-navbar.component.scss Outdated Show resolved Hide resolved
src/lib/vertical-navbar/vertical-navbar.component.ts Outdated Show resolved Hide resolved
src/cdk/a11y/key-manager/list-key-manager.ts Outdated Show resolved Hide resolved
@lskramarov
Copy link
Contributor

И еще, я предполагал, что в итоге добавим компонент menu и поскольку он так же реализует все возможности dropdown, то dropdown в итоге будет удален.

Сейчас dropdown реализует возможности menu и вам не кажется это странным ? dropDOWN будет выпадать вправо/влево ?

@mikeozornin mikeozornin self-requested a review April 18, 2019 20: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.

Гуд!

@rt-gavrilov rt-gavrilov force-pushed the feat/vert-navbar branch 2 times, most recently from d07386a to 7cd4eca Compare April 23, 2019 10:06
@pimenovoleg pimenovoleg merged commit 01715a0 into positive-js:master May 7, 2019
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.

6 participants