feat(plasma-new-hope): Popup added #819
Conversation
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
|
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-819/ |
kayman233
force-pushed
the
kayman233/popup-new-hope.PLASMA-2068
branch
from
069ec79 to
110d90e
Compare
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
kayman233
requested review from
Yeti-or,
neretin-trike and
Yakutoc
as code owners
|
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-819/ |
Почему тут |
| @@ -533,6 +534,10 @@ export { ElasticGrid } | |||
|
|
|||
| export { ElasticGridProps } | |||
|
|
|||
| export { endAnimationClass } | |||
There was a problem hiding this comment.
мы не хотим это за назвать
- class => className
- добавить Popup в начале, или это будут стандартные классы для всех комопнент и анимаций?
There was a problem hiding this comment.
по первому не уверен, что надо добавлять Name
по второму, сложно сказать. В каких у нас еще компонентах есть похожий механизм анимации - при появлении/исчезновении. Вообще наверное ничего не мешает их сделать едиными
я бы еще других ребят послушал насчет этих идей
There was a problem hiding this comment.
There was a problem hiding this comment.
хм, ну по первому я сам везде писал просто class без Name, т.к. по сути это и так читается.
По второму пункту я согласен с Васей, и нужно указать принадлежность к компоненту, потому что мы пока действительно не знаем, общий ли это будет механизм
There was a problem hiding this comment.
А мы можем это вообще не делать API сейчас публичным?
| animation: fadeOut 1s forwards; | ||
| } | ||
|
|
||
| @keyframes fadeIn { |
There was a problem hiding this comment.
эти названия же глобальные?
мы беспокоимся по этому поводу?
There was a problem hiding this comment.
а они внутри сторибука
тогда не паримся
There was a problem hiding this comment.
да, они по сути просто пользовательские
| padding: 1rem; | ||
| `; | ||
|
|
||
| const StyledPopupAnimation = styled(Popup)` |
There was a problem hiding this comment.
Сейчас это выглядит конечно как бойлерплейт который пользователям придется писать у себя =/
Завел issue на подумать про анимацию:
#822
сейчас предлагаю пока пожить с таким решением
| @@ -76,40 +76,42 @@ export function App() { | |||
|
|
|||
| ## Подключение анимации | |||
|
|
|||
| Подключение анимации аналогично тому, как это происходит в `PopupBase`(используя `usePopupAnimation`). | |||
| Подключение анимации аналогично тому, как это происходит в `PopupBase` - через свойство `withAnimation`(управление через `endAnimationClass`/`endTransitionClass`). | |||
There was a problem hiding this comment.
А у нас давно Modal в документации plasma-ui ?
There was a problem hiding this comment.
когда Confirm добавили, я предлагал убрать, если не хочется, но кажется ответа так и не получил тогда :(
There was a problem hiding this comment.
давай уберем таки, ну еще давай в понедельник поднимем вопрос еще раз про plasma-ui и модалки
| export { PopupBase } from '@salutejs/plasma-hope'; | ||
| export { usePopup } from '@salutejs/plasma-hope'; | ||
| export { PopupBaseRoot } from '@salutejs/plasma-hope'; | ||
| export { popupBaseRootClass, endAnimationClass, endTransitionClass } from '@salutejs/plasma-hope'; |
There was a problem hiding this comment.
@kayman233 А мы можем в один export все сделать? или в твоем подходе есть какая то логика?
There was a problem hiding this comment.
да можно, просто мне казалось, что у нас во многих других компонентах подход по типу того, что в core из разных файлов эскпортит, а в условном hope аналогично, но только из одного пакета core(собственно как тут)
There was a problem hiding this comment.
а можешь подсказать, где у нас так сделано? Просто мне тоже кажется, что смысла в подобном разделении особо нет
There was a problem hiding this comment.
например в plasma-web: Card, Carousel, Notification, PaginationDots
There was a problem hiding this comment.
спасиб. Есть ощущение, что это просто ошибка, которая возникла из-за реплейса, когда зависимости с plasma-core и внутренних компонент меняли тупо не глядя на "plasma-hope". Скорее всего там везде нужно это править
There was a problem hiding this comment.
я что-то перестал понимаю где и что надо поправить, но вы это
issue заведите плиз )
и в целом у нас практики написания кода как будто так быстро меняются что мы не успеваем их нигде зафиксировать, ну либо просто нам по кайфу решать как писать код по ходу написания %)
| @@ -2,4 +2,4 @@ export { ModalBase } from '@salutejs/plasma-hope'; | |||
| export { useModal } from '@salutejs/plasma-hope'; | |||
| export { ModalOverlay, modalBaseOverlayClass } from '@salutejs/plasma-hope'; | |||
|
|
|||
| export type { ModalBaseProps, ModalAnimationInfo, ModalBaseRootProps, ModalOverlayProps } from '@salutejs/plasma-hope'; | |||
| export type { ModalBaseProps, ModalBaseRootProps, ModalOverlayProps } from '@salutejs/plasma-hope'; | |||
There was a problem hiding this comment.
[Для инфы]
Это конечно мало вероятно, но удалив ModalAnimationInfo мы "сломаем" обратную совместимость тем кто уже опирается на этот API. В общем это скорее для обратить наше внимание )
There was a problem hiding this comment.
да, сломаем. Но пока я сомневаюсь, что кто-то вообще уже использовал Modal/Popup у себя
There was a problem hiding this comment.
я согласен что ломать не хорошо, но это совсем свежее API, давайте поймем как нам избежать таких ситуаций в дальнейшем
|
|
||
| export interface PopupInfo { | ||
| id: string; | ||
| info?: Object; |
There was a problem hiding this comment.
Object лучше заменить на Record<string, never> || Record<string, any>
У нас даже есть правило Eslint и note - "// NOTE: If you want a type meaning "empty object", you probably want Record<string, never> instead"
'@typescript-eslint/ban-types': [
'warn',
{
extendDefaults: true,
types: {
'{}': false,
object: false,
},
},
],
There was a problem hiding this comment.
вот такой тип имеешь в виду? Record<string, never> | Record<string, any>
There was a problem hiding this comment.
Не-не или то, или другое )
Мы когда с @neretin-trike обсуждали это сошлись что Record<string, any> будет норм.
There was a problem hiding this comment.
кажется лучше Record<string, any> использовать
| * Класс корневого компонента PopupRoot: `popup-base-root` | ||
| */ | ||
| export const popupRootClass = 'popup-base-root'; | ||
| export const endAnimationClass = 'popup-end-animation'; |
There was a problem hiding this comment.
[На обсуждение]
Больно будет переименовать на animationEndClass?
There was a problem hiding this comment.
просто и так переменные на end начинались, поэтому оставил так. По-моему и так, и так нормально
|
|
||
| export interface PopupHookArgs extends Pick<PopupProps, 'isOpen' | 'popupInfo' | 'withAnimation'> { | ||
| id: string; | ||
| } |
There was a problem hiding this comment.
кажется, что было проще не использовать здесь extends и Pick, т.к. по сути ты можешь указать эти поля, и кажется, что выглядело чуть более аккуратно. Но с другой стороны тогда потеряются комментарии и гибкость 🤔 Наверное хочется это обсудить в рамках формирования нашего style-guide
There was a problem hiding this comment.
Я тоже за то что бы обсудить и понять когда/где лучше использовать ts utils, а когда старое доброе дублирование )
There was a problem hiding this comment.
ну вот да, я в целом такой подход использовал для комментов. Но в этом конкретном случае мб и не так нужно, так как наружу этот тип не выносим по идее
| export const endAnimationClass = 'popup-end-animation'; | ||
| export const endTransitionClass = 'popup-end-transition'; | ||
|
|
||
| export const DEFAULT_Z_INDEX = 9000; |
There was a problem hiding this comment.
ещё хотелось бы по аналогии с hooks перенести это в utils/index.ts
There was a problem hiding this comment.
а где у нас итоговый пример по структуре папок/файлов?
kayman233
force-pushed
the
kayman233/popup-new-hope.PLASMA-2068
branch
from
644f194 to
862f06d
Compare
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
kayman233
force-pushed
the
kayman233/popup-new-hope.PLASMA-2068
branch
from
862f06d to
e6002e8
Compare
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
// Poppup.tokens.ts
export const classes = {
startAnimation: 'foo',
endAnimation: 'bar',
};
// Popup.index.ts
export { classes as popupClaseses };// Modal.tsx
import { popupClasses } from '@salutejs/plasma-new-hope';
popupClasses.endAnimation |
|
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-819/ |
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
|
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-819/ |
kayman233
force-pushed
the
kayman233/popup-new-hope.PLASMA-2068
branch
from
c25ee87 to
b634226
Compare
|
Theme Builder app deployed! http://plasma.sberdevices.ru/pr/plasma-theme-builder-pr-819/ |
|
Documentation preview deployed! website: http://plasma.sberdevices.ru/pr/pr-819/ |
|
🚀 This PR is included in version: @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected] 🚀 |
|
🚀 This PR is included in version: @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected] 🚀 |
1 similar comment
|
🚀 This PR is included in version: @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected] 🚀 |
|
🚀 This PR is included in version: @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected], @salutejs/[email protected] 🚀 |
Добавлен компонент
Popupвplasma-new-hope, аналогичныйPopupBaseв других пакетах.Появилась возможность выбирать
frameчерезstring- id элемента.Изменен API для создания анимаций в этом компоненте. Теперь нужно использовать свойство
withAnimation?: boolean(falseпо умолчанию), а для управления использовать классы через переменныеendAnimationClass/endTransitionClass. Пример:fixes #589
⚡ Component performance testing
Result: 🟢 OK
⚡ Component performance testing
Result: 🟢 OK
🐤 Download canary assets:
📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via: