feat(app): create DeleteRecordsModal#21856
Conversation
This PR builds a flexible DeleteRecordsModal component for confirming deleting batches of run
records or compliance log periods. This modal is triggered in a few places- 1) from the Recent
Protocols Run tab of a device's details ('allRuns' variant), 2) from the Compliance Ready Softare
files section of a robot's file manager settings page (for compliance ready robots only, 'allLogs'
variant), and 3) from the Prtoocol Run Records section of a robot's file manager settings page
('selectedRuns' variant).
Closes EXEC-2801
|
|
||
| import type { DeleteRecordsType } from '../types' | ||
|
|
||
| export function useDeleteRecordsText(type: DeleteRecordsType): { |
There was a problem hiding this comment.
maybe tweaking the name since this hook doesn't delete anything 🤔
and this can be an util function can't it?
There was a problem hiding this comment.
I see what you mean regarding the name— the "Delete" comes from the name of the component that this hook is relevant to.
I used a hook since we directly call useTranslation here. I suppose we can keep it as a util, but that would require passing in the translation object in as an arg. I don't feel strongly about this either way, but I think there's precedent for this pattern in protocol-designer/src/components/organisms/AnnouncementModal/announcements.tsx
There was a problem hiding this comment.
honestly don’t think the implementation in protocol-designer/src/components/organisms/AnnouncementModal/announcements.tsx is something we should treat as a recommended pattern. The reason is that it’s structured in a way where code just keeps getting added to the component, even though the component’s core functionality itself isn’t actually changing.
| @@ -0,0 +1 @@ | |||
| export type DeleteRecordsType = 'allRuns' | 'selectedRuns' | 'allLogs' | |||
There was a problem hiding this comment.
nit
defining this in the modal component?
There was a problem hiding this comment.
I can do that, but I moved the hook that also uses this type to its own file, so I thought it might be better to have a types.ts file
| alignItems={ALIGN_CENTER} | ||
| gridGap={SPACING.spacing8} | ||
| justifyContent={JUSTIFY_SPACE_BETWEEN} | ||
| marginBottom="6rem" |
There was a problem hiding this comment.
do you think you can use padding? margin may be a blocker for responsiveness and the design never uses margin.
There was a problem hiding this comment.
Yep, I can do that. I was trying to update this file while making minimal changes to it. It could probably use a larger refactor, but not sure we want to do that here.
There was a problem hiding this comment.
if possible, switching from styled-components to CSS Modules would be great.
- switch
LegacyStyledTexttoStyledText
There was a problem hiding this comment.
Yep, let me do a larger refactor on all the Flexs
| display: flex; | ||
| width: 100%; | ||
| flex-direction: column; | ||
| gap: 0.25rem; |
| padding: 0.5rem; | ||
| margin-right: 12%; | ||
| color: var(--grey-60); | ||
| gap: 1.25rem; |
2 participants
Overview
This PR builds a flexible DeleteRecordsModal component for confirming deleting batches of run records or compliance log periods. This modal is triggered in a few places:
I also wire up this modal in Recent Protocol Runs for smoke testing that variant.
Closes EXEC-2801
Test Plan and Hands on Testing
onConfirmhandler is not yet wired up)Changelog
DeleteRecordModalcomponent, types, tests, and hook for switching text on variantRecentProtocolRunsfor minimal smoke testingReview requests
see test plan
Risk assessment
⬇️