feat(content-uploader): Implement cancel all confirmation modal#4579
feat(content-uploader): Implement cancel all confirmation modal#4579dealwith wants to merge 1 commit into
Conversation
The Cancel All button on the modernized uploads manager now opens a confirmation dialog instead of canceling immediately. Confirming runs handleCancelAllUploads; dismissing leaves uploads untouched. Modal uses @box/blueprint-web AlertModal with localized copy and danger-styled primary action.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
dealwith
changed the base branch from
master
to
implement-cancelled-state-uploads-manager
dealwith
changed the title
| import Footer from './Footer'; | ||
| import UploadsManager from './UploadsManager'; | ||
| import { mapToModernizedUploadItems } from './utils/mapToModernizedUploadItem'; | ||
| import CancelAllUploadsModal from './CancelAllUploadsModal'; |
There was a problem hiding this comment.
[nit] imports should follow alphabetical order
| * passed to the modernized uploads manager so the action requires explicit | ||
| * confirmation before destroying in-progress uploads. | ||
| */ | ||
| handleCancelAllRequest = () => { |
There was a problem hiding this comment.
[nit] This function name is quite misleading because it's not actually handling any (normally network) request. Can we change this to ex. handleCancelAllClick or something implies this is simply opening the modal?
| renderModal(); | ||
| expect(await screen.findByRole('alertdialog')).toBeInTheDocument(); | ||
| expect(screen.getByText('Cancel all uploads?')).toBeInTheDocument(); | ||
| expect(screen.getByText(/Files that are still uploading will be canceled/)).toBeInTheDocument(); |
There was a problem hiding this comment.
[nit] Is there a reason why this can not be expected with the literal string and has to be regex?
| defaultMessage: 'Cancel All', | ||
| description: 'Confirm button for the cancel all uploads modal', | ||
| }, | ||
| cancelAllUploadsKeepButton: { |
There was a problem hiding this comment.
The name feels slightly unclear what it's trying to do. How about keepUploadingButton?
| @@ -0,0 +1,31 @@ | |||
| import { defineMessages } from 'react-intl'; | |||
|
|
|||
There was a problem hiding this comment.
[question] What's the intended scope of this file?
- If it's dedicated to CancelAllUploadsModal, the prefix on every key is redundant — heading, body, confirmButton, dismissButton, closeLabel would read cleaner.
- If it'll hold messages for other modals in content-uploader/, dropping just Modal (already implied) gets you cancelAllUploadsHeading, cancelAllUploadsBody, etc.
- If it'll hold non-modal messages too, the current cancelAllUploadsModal prefix makes sense as-is.
| @@ -1195,6 +1198,24 @@ class ContentUploader extends Component<ContentUploaderProps, State> { | |||
| * Cancel every pending or in-progress upload at once. Items keep their row | |||
There was a problem hiding this comment.
should this be further down with handleCancelAllUploads? or, should your inserted code be above this comment block?
| instance.handleCancelAllConfirm(); | ||
|
|
||
| expect(wrapper.state('isCancelAllModalOpen')).toBe(false); | ||
| expect(inProgress.status).toBe('canceled'); |
4 participants
4/5 PR in the queue: