Announcements #26
Announcements #26
Conversation
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Florent MILLOT <[email protected]>
src/components/App/app-top-bar.tsx
Outdated
| [ | ||
| MainPaths.announcements, | ||
| <TabNavLink | ||
| icon={<Announcement />} | ||
| label={<FormattedMessage id="appBar.tabs.announcement" />} | ||
| href={`/${MainPaths.announcements}`} | ||
| value={MainPaths.announcements} | ||
| key={`tab-${MainPaths.announcements}`} | ||
| />, | ||
| ], |
There was a problem hiding this comment.
Can we avoid storing ReactElement in a variable ?
There was a problem hiding this comment.
I just use the existing pattern, if you think we should refactor this, we need to do an other PR.
How bad is it ?
| <Grid item> | ||
| <List sx={{ maxWidth: 1400 }}> | ||
| {announcements.map((announcement) => ( | ||
| <ListItemAnnouncement {...announcement} /> |
There was a problem hiding this comment.
AnnouncementItem ? And I think I prefer when the arguments are an object.
<AnnouncementItem item={announcement} /> ? I think it makes it more readable
There was a problem hiding this comment.
I prefer like this, for me this way is clearer.
export const ListItemAnnouncement: FunctionComponent<Announcement> = (
No need to define a new ListItemAnnouncementProps with just one item which is an Announcement...
It's already an item itself, so why giving an inner item prop ?
It follow the pattern ListItemText.
But if it's rly a bad way and if the second reviewer agree with you, I'll change it.
| <Grid item container spacing={2} direction="column"> | ||
| <Grid item> | ||
| <AppBar position="static" color="default"> | ||
| <Toolbar variant="dense"> | ||
| <Button | ||
| onClick={() => setOpenDialog(true)} | ||
| variant="outlined" | ||
| startIcon={<AddCircleOutline />} | ||
| size="small" | ||
| > | ||
| {intl.formatMessage({ id: 'announcements.add' })} | ||
| </Button> | ||
| </Toolbar> | ||
| </AppBar> | ||
| </Grid> |
There was a problem hiding this comment.
Can you extract that in common component with Users ?
|
|
||
| return ( | ||
| //@ts-ignore because RHF TS is broken | ||
| <FormProvider validationSchema={formSchema} {...formMethods}> |
There was a problem hiding this comment.
Are you sure ? It's not just that we shouldn't define this property ? Schema is in the yup resolver already, no ?
There was a problem hiding this comment.
It's something we already talked about on Slack.
We found some way to avoid the issue, ie
https://github.com/gridsuite/gridstudy-app/blob/760264391c6b49a26e60edb94973b5752dbf9d67/src/components/dialogs/dynamicsimulation/event/dynamic-simulation-event-dialog.tsx#L228-L233
But not working here
There was a problem hiding this comment.
Actually I'm wondering if we don't do it wrong since the start haha
| import { useAnnouncements } from './useAnnouncements'; | ||
| import { ListItemAnnouncement } from './ListItemAnnouncement'; | ||
|
|
||
| const Announcements: FunctionComponent = () => { |
There was a problem hiding this comment.
Maybe we can separate in different components the top bar from the announcements list
There was a problem hiding this comment.
I'm extracting the Topbar/Toolbar combo, you think about something else ?
| }, | ||
| }; | ||
|
|
||
| // const formSchema: ObjectSchema<FormData> = yup |
There was a problem hiding this comment.
Yeah I've let it for the reviewers to discuss about this issue because I don't really like all this, with the tsignore also...
| [HOURS, MINUTES], | ||
| [DAYS, MINUTES], | ||
| [DAYS, HOURS], |
There was a problem hiding this comment.
How does that work ?
There was a problem hiding this comment.
Internally I don't really know, it's not even in the doc, but like this we remove all cyclic dep combo
| }; | ||
|
|
||
| // const formSchema: ObjectSchema<FormData> = yup | ||
| export const formSchema = yup |
There was a problem hiding this comment.
Can we separate the formSchema from type definitions ?
There was a problem hiding this comment.
What do you mean ?
| @@ -95,3 +100,44 @@ export function addUser(sub: string): Promise<void> { | |||
| throw reason; | |||
| }); | |||
| } | |||
|
|
|||
| export function getAnnouncements(): Promise<AnnouncementServerData[]> { | |||
There was a problem hiding this comment.
Can we catch errors and display snackBar ?
There was a problem hiding this comment.
Yes I forgot you're right
src/pages/announcements/utils.ts
Outdated
| { | ||
| [DAYS]: getDurationUnitSchema(HOURS, MINUTES), | ||
| [HOURS]: getDurationUnitSchema(DAYS, MINUTES), | ||
| [MINUTES]: getDurationUnitSchema(DAYS, HOURS), | ||
| }, | ||
| // to avoid cyclic dependencies | ||
| [ | ||
| [HOURS, MINUTES], | ||
| [DAYS, MINUTES], | ||
| [DAYS, HOURS], | ||
| ] | ||
| ) | ||
| .required(), | ||
| }) | ||
| .required(); | ||
|
|
||
| function getDurationUnitSchema(otherUnitName1: string, otherUnitName2: string) { | ||
| return yup.number().when([otherUnitName1, otherUnitName2], { | ||
| is: (v1: number | null, v2: number | null) => | ||
| v1 === null && v2 === null, | ||
| then: (schema) => schema.required(), | ||
| otherwise: (schema) => schema.nullable(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can't we simplify by saying at least one of these three is required ?
There was a problem hiding this comment.
I don't see how. We can do that for the validation but then the fields won't be red with "required" key word.
| export const formSchema = yup | ||
| .object() | ||
| .shape({ | ||
| [MESSAGE]: yup.string().max(100).required(), |
There was a problem hiding this comment.
100 is quite limiting, maybe double check with a PO
Signed-off-by: Florent MILLOT <[email protected]>
Signed-off-by: Abdelsalem <[email protected]>
jonenst
force-pushed
the
main
branch
2 times, most recently
from
35af5e1 to
d93158a
Compare
Add the possibility to send an announcement to the connected users of other web app for a specified duration.