Changed "published" button in automations header#27852
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR changes AutomationHeader to accept an AutomationDetail object (automation: AutomationDetail | undefined) and isLoading, deriving name and isActive from automation. While loading it shows a Skeleton; when active it renders an ellipsis dropdown with a "Turn off" item and sets the publish button disabled and labeled "Published". AutomationEditor now passes automation={automation} and isLoading to AutomationHeader. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/posts/src/views/Automations/components/automation-header.tsx (2)
39-43: TODO: Implement "Turn off" functionality.The "Turn off" menu item is currently a placeholder. This should be implemented as part of NY-1267.
Would you like me to open a new issue to track the implementation of the "Turn off" functionality, or is this already tracked in NY-1267?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/posts/src/views/Automations/components/automation-header.tsx` around lines 39 - 43, The "Turn off" DropdownMenuItem is only a placeholder; wire it to actual turn-off logic by adding a click handler to DropdownMenuItem that calls the existing turn-off flow (e.g., invoke a provided prop like onTurnOff or dispatch the turnOffAutomation/disableAutomation action using the automation id available in AutomationHeader), ensure you import/use the mutation or action used elsewhere (e.g., disableAutomation or useDisableAutomationMutation) and handle UI feedback (disable state/spinner and error handling); optionally show a confirmation dialog before calling the turn-off function and update props/signature of AutomationHeader (or its parent) to accept an onTurnOff callback if one doesn’t already exist.
47-52: Verify publish button behavior matches product requirements.The button is now:
- Disabled when
isLoading || isActive- Shows "Published" when active, "Publish changes" otherwise
This matches the PR description: "The previous 'Published' button is now disabled and shows different text." However, the TODO comment suggests the button action itself needs implementation.
Would you like me to open a new issue to track the publish button functionality implementation, or is this already covered by NY-1267?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/posts/src/views/Automations/components/automation-header.tsx` around lines 47 - 52, The publish Button in AutomationHeader is only UI-disabled and labeled but lacks an action; remove or replace the TODO by wiring the button to the real publish flow (attach an onClick handler that dispatches/sends the publish action used elsewhere in the app, e.g., call the publishAutomation/publishDraft function or trigger the relevant Redux/Query mutation) while keeping the disabled logic (disabled={isLoading || isActive}) and text toggle using isActive; if the publish implementation is not yet available, create/associate a tracking issue (NY-1267) and add a clear TODO referencing that issue and a stubbed onClick that no-ops until the publish API is implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/posts/src/views/Automations/components/automation-header.tsx`:
- Around line 34-36: The ellipsis icon Button in the AutomationHeader component
lacks an accessible label; update the Button (the element rendering
<LucideIcon.Ellipsis />) to include an aria-label (e.g., aria-label="More
actions" or aria-label="Open menu") so screen readers know its purpose and it
remains size='icon' variant='ghost'; add the aria-label prop to that Button
component where it is declared.
---
Nitpick comments:
In `@apps/posts/src/views/Automations/components/automation-header.tsx`:
- Around line 39-43: The "Turn off" DropdownMenuItem is only a placeholder; wire
it to actual turn-off logic by adding a click handler to DropdownMenuItem that
calls the existing turn-off flow (e.g., invoke a provided prop like onTurnOff or
dispatch the turnOffAutomation/disableAutomation action using the automation id
available in AutomationHeader), ensure you import/use the mutation or action
used elsewhere (e.g., disableAutomation or useDisableAutomationMutation) and
handle UI feedback (disable state/spinner and error handling); optionally show a
confirmation dialog before calling the turn-off function and update
props/signature of AutomationHeader (or its parent) to accept an onTurnOff
callback if one doesn’t already exist.
- Around line 47-52: The publish Button in AutomationHeader is only UI-disabled
and labeled but lacks an action; remove or replace the TODO by wiring the button
to the real publish flow (attach an onClick handler that dispatches/sends the
publish action used elsewhere in the app, e.g., call the
publishAutomation/publishDraft function or trigger the relevant Redux/Query
mutation) while keeping the disabled logic (disabled={isLoading || isActive})
and text toggle using isActive; if the publish implementation is not yet
available, create/associate a tracking issue (NY-1267) and add a clear TODO
referencing that issue and a stubbed onClick that no-ops until the publish API
is implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16bd2608-f046-4d74-bff4-569b314f95aa
📒 Files selected for processing (2)
apps/posts/src/views/Automations/components/automation-header.tsxapps/posts/src/views/Automations/editor.tsx
towards https://linear.app/ghost/issue/NY-1267 If the automation is active, there's now a little dropdown with a "Turn off" button. The "Published" button is now disabled and has different text. If the automation is inactive, the button looks the same as it did before.
EvanHahn
force-pushed
the
published-button-in-automations-header
branch
from
e4a9321 to
6af43ce
Compare
| interface AutomationHeaderProps { | ||
| name?: string; | ||
| isLoading?: boolean; | ||
| automation: undefined | AutomationDetail; |
There was a problem hiding this comment.
nit: it looks weird to me for undefined (or null) to be first. unless there's a reason to?
There was a problem hiding this comment.
If it looks weird to you, I'll change it. Done in 76cc302.
I have a "bad news first" coding style. undefined | AutomationDetail instead of AutomationDetail | undefined, handling the sad path before the happy path, testing errors before successes, that sort of thing. I find it pushes me to think about error cases, which I think improves the reliability of software. But it's a small thing, especially here.
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align='end'> | ||
| {/* TODO(NY-1267) Make this button do something */} |
There was a problem hiding this comment.
light suggestion: it could make sense to add the "are you sure" modal now if you want, then we only need to worry about wiring the modal's button to the endpoint once it's ready. could be a followup or in the same PR as the endpoint wiring though
There was a problem hiding this comment.
I considered this but it's pretty coupled to the edit state, so I didn't add this yet.
| )} | ||
| <Button | ||
| // TODO(NY-1267) Make this button do something | ||
| disabled={isLoading || isActive} |
There was a problem hiding this comment.
from the screenshot in the description, it looks like the disabled state of the button looks slightly different than it does from the prototype in the ticket. but i think ideally the disabled state is something that's baked in to the component, and we're not setting it separately. just noting we'll want to verify this (linking this comment to the ticket for now).
There was a problem hiding this comment.
Saw the same thing. I don't think we should have a special button just for this, but Zimo/Peter can disagree.
2 participants
towards https://linear.app/ghost/issue/NY-1267
If the automation is active, there's now a little dropdown with a "Turn off" button. The "Published" button is now disabled and has different text.
If the automation is inactive, the button looks the same as it did before.
The buttons don't actually do anything yet—that's coming soon!