Skip to content

feat(tabs): component Tabs #46

Merged
merged 27 commits into from
Dec 17, 2018
Merged

Conversation

Aden-git
Copy link
Contributor

@Aden-git Aden-git commented Sep 17, 2018

Еще предстоит:

  • Документация

  • Ховер должен иметь закругленный бордер

  • Табы должны быть 40px в высоту

  • border-radius у табов должен быть 3px (сейчас 2px)

  • Текст на дизебленной вкладке ярче чем надо, должен быть бледный.

  • При клике не должен появляться фокус

  • border не должен менять цвет в disabled state

  • стиль hover не должны не нужен на выбранном табе.

  • lint

  • БЭМ

  • ? Удалить возможность анимированного переключения контента?

  • ? Клавиатурная навигация: переключать контент сразу или по нажатию Enter/Space ?

@lskramarov
Copy link
Contributor

Напомните, в каком состоянии сейчас компонент ?

@Aden-git
Copy link
Contributor Author

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

@pimenovoleg
Copy link
Member

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

необходимость есть, high priority

@Aden-git
Copy link
Contributor Author

необходимость есть, high priority

Ок, тогда к понедельнику доведу PR

@pimenovoleg pimenovoleg requested review from lskramarov, mikeozornin and pimenovoleg and removed request for mikeozornin October 29, 2018 08:27
@Aden-git Aden-git changed the title [WIP] feat(tabs): component Tabs feat(tabs): component Tabs Oct 31, 2018
@Aden-git Aden-git changed the title feat(tabs): component Tabs [WIP] feat(tabs): component Tabs Oct 31, 2018
@mikeozornin
Copy link
Contributor

http://d.mikeozornin.ru/X7Rd2T
Возможно это не к табам. При переключении табов контент двигается.
Пример с Very slow animation вообще вреден. Табы себя так не ведут. Табы просто быстро переключают контент, без сдвигов.

Хувер таба прямоугольный, а должен быть такой же формы, как фон выбранной вкладки.

http://d.mikeozornin.ru/sJYeXJ
Вкладки очень высокие, должны быть 40 пк.

border-radius у табов 3 пк, а не 2 пк.

Мы не ставим фокус на элементы, если его кликнули мышкой, фокус ставится только если нажимаем клавиатурой. Сейчас при клике на таб у него зажигается рамка фокуса, а не должна. Правильное поведение можно посмотреть в кнопках в мастере.

Таска в package.json не на своем месте
"server-dev:tabs": "npm run server-dev -- --env.component tabs",
Лучше их делать по алфавиту, проще будет найти.

http://d.mikeozornin.ru/7tbMR6
Возможно это странный пример, но тут вкладки не выбираются. Фокус ставится, а переключение не происходит.

Текст на дизебленной вкладке ярче чем надо, должен быть бледный.

@Aden-git
Copy link
Contributor Author

Aden-git commented Nov 1, 2018

http://d.mikeozornin.ru/X7Rd2T
Возможно это не к табам. При переключении табов контент двигается.
Пример с Very slow animation вообще вреден. Табы себя так не ведут. Табы просто быстро >переключают контент, без сдвигов.

Это сделано по аналогии с material, управляется опцией, можно выключить: animationDuration="0ms"
Можем сделать 0ms по умолчанию, или выпилить анимации на корню.

@Aden-git
Copy link
Contributor Author

Aden-git commented Nov 1, 2018

Мы не ставим фокус на элементы, если его кликнули мышкой, фокус ставится только если нажимаем клавиатурой. Сейчас при клике на таб у него зажигается рамка фокуса, а не должна. Правильное поведение можно посмотреть в кнопках в мастере.

Я удивлен такому подходу.
Как пользователь будет ориентироваться на странице при клавиатурной навигации? Как понять, какой следующий элемент будет в фокусе при табуляции если был сделан клик на какой нибудь контрол?
Какое поведение должно быть при клике на табы, если они были выбраны при табуляции?

@Aden-git
Copy link
Contributor Author

Aden-git commented Nov 1, 2018

http://d.mikeozornin.ru/7tbMR6
Возможно это странный пример, но тут вкладки не выбираются. Фокус ставится, а переключение не происходит.
Тут действительно распространены с справедливы оба подхода:

  1. Выбор вкладки при фокусировке
  2. Фокусировка и выбор вкладки отдельные понятия. Выбор вкладки происходит по нажатию Enter/Space

Первый подход больше подходит для легких вкладок, со статическим или легким контентом.
Второй подход, как по мне, более универсальный. Если мы используем табы для навигации между вьюшками или контент подгружается с сервера - путь с первой табы до последней при клавиатурной навигации будет затруднен для пользователя загрузкой контента вьюшек или запросами на сервер.
Мы можем добавить дополнительную опцию для управления этими сценариями или оставить текущий вариант. Что думаете?

Copy link
Contributor

@lskramarov lskramarov left a comment

Choose a reason for hiding this comment

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

Нужно обновить документацию https://wiki.ptsecurity.com/display/MOSAIC/Tabs

@lskramarov
Copy link
Contributor

Мы не ставим фокус на элементы, если его кликнули мышкой, фокус ставится только если нажимаем клавиатурой. Сейчас при клике на таб у него зажигается рамка фокуса, а не должна. Правильное поведение можно посмотреть в кнопках в мастере.

Я удивлен такому подходу.
Как пользователь будет ориентироваться на странице при клавиатурной навигации? Как понять, какой следующий элемент будет в фокусе при табуляции если был сделан клик на какой нибудь контрол?
Какое поведение должно быть при клике на табы, если они были выбраны при табуляции?

это поведение реализовано в FocusMonitor, тебе осталось только использовать его, и в стилях заменить .cdk-focused на .cdk-keyboard-focused. Можно посмотреть на другие компоненты.

@lskramarov
Copy link
Contributor

Пытался посмотреть MR, но не пошло, без документации не хочется по коду разбираться, что такое mc-tab-link, mc-tab-nav-bar, mc-light-tabs, mcPreffix и т.д.

@Aden-git
Copy link
Contributor Author

Aden-git commented Nov 6, 2018

это поведение реализовано в FocusMonitor, тебе осталось только использовать его, и в стилях заменить .cdk-focused на .cdk-keyboard-focused. Можно посмотреть на другие компоненты.

Я изменю поведение для консистентности, но мой вопрос больше касается UX.
По моему мнению, сохранение фокуса при клике является базовой концепцией. Может у нас где-то зафиксированы описание причин/итоги диалога принятия такого решения?

@lskramarov
Copy link
Contributor

это поведение реализовано в FocusMonitor, тебе осталось только использовать его, и в стилях заменить .cdk-focused на .cdk-keyboard-focused. Можно посмотреть на другие компоненты.

Я изменю поведение для консистентности, но мой вопрос больше касается UX.
По моему мнению, сохранение фокуса при клике является базовой концепцией. Может у нас где-то зафиксированы описание причин/итоги диалога принятия такого решения?

при клике фокус будет на этом элементе, но он не будет виден и после нажатия на Tab фокус перейдет на следующий элемент и будет отображен.

@Aden-git
Copy link
Contributor Author

Aden-git commented Nov 6, 2018

Пытался посмотреть MR, но не пошло, без документации не хочется по коду разбираться, что такое mc-tab-link, mc-tab-nav-bar, mc-light-tabs, mcPreffix и т.д.

Хорошо, первым делом займусь документацией.

@Aden-git
Copy link
Contributor Author

@lskramarov
API описал, но по доке еще будут правки после разрешения вопросов:
https://wiki.ptsecurity.com/display/MOSAIC/Tabs
Также перечислил known issues в описании PR.

@Aden-git Aden-git changed the title [WIP] feat(tabs): component Tabs feat(tabs): component Tabs Nov 13, 2018
@mikeozornin
Copy link
Contributor

http://screenshots.ptsecurity.com/mozornin-2018-11-14_15-52-50.png
Сейчас при дизебле линия вкладки тоже бледнеет, не должна. Линия под Second.

http://d.mikeozornin.ru/06HdXT
Cейчас есть хувер на выбранном элементе, выбранный элемент повторно не кликается поэтому хувер на нем не нужен.

Остальное все хорошо.

Copy link
Contributor

@lskramarov lskramarov left a comment

Choose a reason for hiding this comment

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

Посмотрел не все, завтра продолжу.

src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-group.ts Outdated Show resolved Hide resolved
src/lib/tabs/tab-header.html Outdated Show resolved Hide resolved
src/lib/tabs/tab-header.scss Show resolved Hide resolved
src/lib/tabs/tab-header.scss Outdated Show resolved Hide resolved
src/lib-dev/tabs/template.html Outdated Show resolved Hide resolved
@lskramarov
Copy link
Contributor

Максим, скажи, у тебя есть время что бы можно было поправить замечания в ближайшие дни ? например завтра или в понедельник ?

Очень не хочется затягивать и ждать еще неделю...

@Aden-git
Copy link
Contributor Author

Aden-git commented Dec 7, 2018

  1. Стас
  2. Сейчас в отпуске, в понедельник только прилечу домой. Смогу поправить во вторник.
  3. Да, затягивать не хочется

@lskramarov
Copy link
Contributor

  1. Стас

Прошу прощения :)

@Aden-git
Copy link
Contributor Author

@lskramarov
Поправил линты, бывают какие-то проблемки у меня с линтерами в VS Code, проверил все консольными утилитами, все должно быть ок.

@pimenovoleg pimenovoleg merged commit ec93802 into positive-js:master Dec 17, 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