fix: some bug fix about publish page #5012
Conversation
There was a problem hiding this comment.
Several advices to make sure readability:
- Avoid nested if else clauses, return early.
- Extract complicated state computation logic as pure functions.
- Define necessary local variables to replace appearances of
botPublishHistory[0].status. Avoid context switching when reading the code
fd109f2 to
242c667
Compare
|
I agree we can merge it if this is functional working and help unblock us for E2E testing. But i do have a few concern about the code quality here, left a few more comments for refactoring. See below |
| @@ -159,49 +159,85 @@ const Publish: React.FC<RouteComponentProps<{ projectId: string; targetName?: st | |||
| }; | |||
|
|
|||
| const [pendingNotification, setPendingNotification] = useState<Notification>(); | |||
| const [previousBotPublishHistoryList, setPreviousBotPublishHistoryList] = useState(botPublishHistoryList); | |||
There was a problem hiding this comment.
can we have a better naming for this what previousHistory means? do we have a current history?
and there is no good to add "list" as postfix, botPublishHistory is good enough
| return item; | ||
| }) | ||
| ); | ||
| if (!(bot.publishTarget && bot.publishTargets)) { |
There was a problem hiding this comment.
First, the comments "// most recent item is a 202, which means we should poll for updates..." is not related to this line. there is even no 202 in many lines.
For such a long use effect and the many other long use effects below, we should give a top-level summary.
* get all publish history && notification cards display * reduce nested loop
Description
bugfix
Task Item
#minor
Screenshots