✨ TopAppBarMenu - add custom menu #193
Conversation
AdrienCasta
force-pushed
the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
11 times, most recently
from
75860c2 to
8a4461b
Compare
AdrienCasta
force-pushed
the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
from
8a4461b to
acb5f8e
Compare
AdrienCasta
force-pushed
the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
from
acb5f8e to
a4b73d9
Compare
| onBack?: () => void; | ||
| style?: ViewStyle; | ||
| action?: ReactNode; |
There was a problem hiding this comment.
en fait ici le client de la lib va pouvoir venir donner directement l'icon + action en tant que child component ?
There was a problem hiding this comment.
Yes c'est l'idée ! On avait besoin de faire plus qu'afficher une icone et gérer onPress. Pour un soucis de simplicité, je redonne la main au consumer de la lib en laissant libre un AppBar.Action.
J'ai rajouté les icone "close" et "printer settings" dans le DS.
<TopAppBar title="" action={<TopAppBar.CloseAction onPress={args.onMenuItemPress} />} />
<TopAppBar title="" action={<TopAppBar.PrinterSettingsAction onPress={args.onMenuItemPress} />} />| TopAppBar.CloseAction = TopAppBarCloseAction; | ||
| TopAppBar.MenuAction = TopAppBarMenuAction; | ||
| TopAppBar.PrinterSettingsAction = TopAppBarPrinterSettingsAction; |
There was a problem hiding this comment.
ici c'est des menus par defaut proposer par la lib ? Car je pense que par exemple le PrinterSettingsAction aurait plus sa place dans l'app directement et pas dans la lib.
There was a problem hiding this comment.
C'est ce qui est fait actuellement en insérant le name de l'icône actuellement, l'icone est côté DS :) J'ai pris la décision de partir garder l'action côté DS également
There was a problem hiding this comment.
J'ai un doute aussi. Si on se projette dans le futur, je ne suis pas sûr que l'on veuille référencer toutes nos actions dans le DS. J'aurais donc mis les actions dans l'application et non dans le DS.
| ); | ||
| }; | ||
|
|
||
| function getMenuItemConfig(theme: Theme, id: TopAppBarMenuItemId) { |
There was a problem hiding this comment.
Je ne vois pas l'intérêt de l'id. Tu peux m'expliquer ?
| }); | ||
| }); | ||
|
|
||
| describe('TopAppBarMenuItem failing to mount', () => { |
There was a problem hiding this comment.
Je ne comprends pas ce test. Je pense que c'est lié à l'id 🤔
| expect(screen.getByText(topBarTitle)).toBeOnTheScreen(); | ||
| }); | ||
|
|
||
| it.todo("testing menu title 'as string' on press"); |
There was a problem hiding this comment.
A implémenter ou supprimer 😉
There was a problem hiding this comment.
Créer un ticket et lier le todo au ticket.
clementdejoie
force-pushed
the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
from
07c1531 to
6b9d2c7
Compare
clementdejoie
force-pushed
the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
from
6b9d2c7 to
1ded399
Compare
AdrienCasta
deleted the
task/rew-287-modifier-la-topappbar-pour-integrer-un-sous-menu
branch
Inspiration :
video.mp4