Conversation
geodem127
changed the title
geodem127
requested review from
agalin920 and
finnar-bin
and removed request for
agalin920
geodem127
force-pushed
the
feature/3151-schedule-unpublish-button
branch
from
4c2e3e7 to
6609be8
Compare
There was a problem hiding this comment.
Is this not duplicating a ton of stuff we use for SchedulePublish? I would imagine there would be a lot of shared code between them
There was a problem hiding this comment.
Refactored SchedulePublish component to support scheduled un-publishing
Coverage ReportOverall Coverage
Changed Files Coverage
|
| if (item?.dirty) { | ||
| return ITEM_STATES.dirty; | ||
| } else if (item?.scheduling?.isScheduled) { | ||
| } else if (item?.scheduling?.isScheduled && !item?.publishing?.publishAt) { |
There was a problem hiding this comment.
why is item?.publishing?.publishAt being added? I would assume isScheduled already covers this.
| { | ||
| users?.find( | ||
| (user: any) => | ||
| user.ZUID === activePublishing?.publishedByUserZUID | ||
| )?.firstName | ||
| }{" "} | ||
| { | ||
| users?.find( | ||
| (user: any) => | ||
| user.ZUID === activePublishing?.publishedByUserZUID | ||
| )?.lastName |
There was a problem hiding this comment.
I would honestly prefer to have this as a variable instead of inline
| { | ||
| users?.find( | ||
| (user: any) => | ||
| user.ZUID === activePublishing?.publishedByUserZUID | ||
| )?.firstName | ||
| }{" "} | ||
| { | ||
| users?.find( | ||
| (user: any) => | ||
| user.ZUID === activePublishing?.publishedByUserZUID | ||
| )?.lastName | ||
| } |
There was a problem hiding this comment.
Same for this one, prefer to have this as a variable
There was a problem hiding this comment.
I'm aware that this file has been this way from the beginning but I honestly find it hard to follow the rendering logic with all the nested ternary operators. Might be worth looking at refactoring this. Thoughts @agalin920 ?
| publishAt: isForUnpublish ? "now" : publishAtUtcStr, | ||
| unpublishAt: isForUnpublish ? publishAtUtcStr : "never", |
There was a problem hiding this comment.
Why do these need to be changed given that this function is supposed to only handle publishing?
| const message = | ||
| (!!data.publishAt && data.publishAt !== "now") || | ||
| (data.publishAt === "now" && | ||
| !!data?.unpublishAt && | ||
| data?.unpublishAt !== "never") | ||
| ? `Scheduled ${title} to ${ | ||
| data.publishAt === "now" && data?.unpublishAt !== "never" | ||
| ? "unpublish" | ||
| : "publish" | ||
| } on ${meta.localTime} in the ${meta.localTimezone} timezone` | ||
| : `Published ${title} now`; |
There was a problem hiding this comment.
Prefer to have this as an if-else for readability. Kinda hard to follow heavily-nested ternary operations like this.
| let actionDispatch = null; | ||
|
|
||
| if (isForUnpublish) { | ||
| actionDispatch = publish( |
There was a problem hiding this comment.
I'm not sure why we're doing a publish action when we're supposed to unpublish something?
3 participants
Screen.Recording.2026-03-26.at.2.20.33.AM.mov