TAN-7385 add scheduling capability to modal#13662
TAN-7385 add scheduling capability to modal#13662IvaKop wants to merge 30 commits intoTAN-7137/project-publication-back-endfrom
Conversation
…com:CitizenLabDotCo/citizenlab into TAN-7385-add-scheduling-capability-to-modal
|
IvaKop
changed the base branch from
TAN-7385-project-scheduling-modal
to
TAN-7137/project-publication-back-end
|
@luucvanderzee Once again, let me know if some additional context is needed. In this one we:
|
There was a problem hiding this comment.
Code looks mostly fine, just some suggestions. But I found the approval flow for PMs quite confusing.
As a PM, I request an approval for a project to be published at a scheduled time (see image below):
As the admin who is reviewing this, I expected then to see a message saying something like: "Hey, the PM who requested your review suggested the following time to publish this project. Ok or not?". But instead, I got just the normal modal without any indication of the time that the PM has selected:
If the PM requested the project to be published directly, instead of scheduled, the behavior is also kind of confusing, because as admin you can't see whether the PM requested a direct publication of a scheduled publication. But when a scheduled publication is requested, it's even worse. I think this needs to be solved before releasing this feature.
From the perspective of the admin looking at the review request, I would expect to see something like the following:
- A summary of what was requested exactly: who requested the approval, if publication is scheduled or immediately, and the discoverability and recipients settings.
- If the publication is scheduled: explain for when it is scheduled, and let the admin know through a warning if the scheduled date is already in the past (since it might take some time before the admin / FM gets around to reviewing it). If the publication is immediate: explain implications of approving this (the project goes live immediately)
- Make sure that the admin first has to either explicitly approve or explicitly decline before they are allowed to make further changes to the publication status, the scheduling or the discoverability/email recipient settings. Only show the current modal with the full settings after they explicitly decline. Ideally, the PM should get a notification if the thing gets either accepted or declined.
Also, maybe out of scope, but I kind of expected to see something more in my face as an admin. When I filtered the projects list by 'required approval', I kind of expected something like: when I click on the project that requires approval, it's immediately going to be obvious this means and what I need to do. But there was nothing. From the code I knew that I had to go to the publication status thing in the top right, but if you don't know that, I think you will be pretty lost as an admin
| color: getStatusColor(project.attributes.publication_status), | ||
| color: getStatusColor( | ||
| project.attributes.publication_status, | ||
| project.attributes.publication_status === 'draft' && |
There was a problem hiding this comment.
Projects can only have scheduled_at if they are in draft, right? So maybe that check here is redundant?
| const { publication_status, visible_to, listed } = project.attributes; | ||
| const { publication_status, visible_to, listed, scheduled_at } = | ||
| project.attributes; | ||
| const isScheduled = publication_status === 'draft' && !!scheduled_at; |
| status: PublicationStatus, | ||
| isScheduled = false | ||
| ) => { | ||
| if (isScheduled) return '#7C3AED'; |
There was a problem hiding this comment.
Maybe we can use a color from the design system here?
| {isScheduled && ( | ||
| <Text m="0" fontSize="s"> | ||
| ({formatMessage(sharedMessages.scheduled)}) | ||
| </Text> |
There was a problem hiding this comment.
I like this addition, maybe we can also put the date when the publication is scheduled in the tooltip?
| const { data: appConfiguration } = useAppConfiguration(); | ||
| const timezone = | ||
| appConfiguration?.data.attributes.settings.core.timezone ?? ''; | ||
| const tenantTimeNow = timezone ? moment().tz(timezone).toDate() : new Date(); |
There was a problem hiding this comment.
Can we use date-fns here?
3 participants
Changelog
Added
For translators