feat: add avoidChangesNotSavedAlertOnClose prop #541
Conversation
WalkthroughThe Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx (2)
20-20: Consider simplifying the destructuring pattern.The current pattern destructures
configtwice, which could be confusing. Consider using direct property access or a cleaner destructuring approach.Apply this diff for a cleaner approach:
- config: { onClose, avoidChangesNotSavedAlertOnLeave }, + config: { onClose, avoidChangesNotSavedAlertOnLeave, ...restConfig },Or alternatively, use direct property access in the function:
- config: { onClose, avoidChangesNotSavedAlertOnLeave },And reference it as
config.avoidChangesNotSavedAlertOnLeavein theopenModalfunction.
47-54: Logic implementation is correct, but consider renaming for clarity.The conditional logic correctly implements the required functionality to skip the modal when
avoidChangesNotSavedAlertOnLeaveis true. However, the function nameopenModalis now misleading since it might not open the modal at all.Consider renaming the function to better reflect its behavior:
- const openModal = () => { + const handleModalAction = () => { // Don't open the modal if avoidChangesNotSavedAlertOnLeave is true if (avoidChangesNotSavedAlertOnLeave) { closeWithReason(); return; } setIsModalOpened(true); };And update the reference on line 59:
- onClick: isResetted ? closeWithReason : openModal, + onClick: isResetted ? closeWithReason : handleModalAction,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx(2 hunks)
hamza221
force-pushed
the
fix/discard-confirmation-modal-on-config
branch
2 times, most recently
from
15c00a6 to
d84942c
Compare
|
@hamza221 this is not a bug, and this needs a different prop if you want to make it configurable, the alert here means the browser's alert for sudden leaving the page. |
hamza221
force-pushed
the
fix/discard-confirmation-modal-on-config
branch
from
d84942c to
1035cfe
Compare
hamza221
changed the title
Thank you for the feedback, switched to a feature commit |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
500-509: Minor wording & comma for readability-By default once the user makes any change/edit on the image and hasn't saved the image yet and then clicks on the close button, a confirmation modal will be shown asking if they really want to close without saving. Setting this to `true` will skip showing the confirmation modal and close immediately. +By default, once the user has unsaved changes and clicks the close button, a confirmation modal is shown asking whether they really want to close without saving. Setting this to `true` skips the modal and closes immediately.🧰 Tools
🪛 LanguageTool
[uncategorized] ~507-~507: Did you mean: “By default,”?
Context: ...: +v4.0.0 Default:falseBy default once the user makes any change/edit on ...(BY_DEFAULT_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx(2 hunks)packages/react-filerobot-image-editor/src/context/defaultConfig.js(1 hunks)packages/react-filerobot-image-editor/src/index.d.ts(1 hunks)public/demo-config.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/react-filerobot-image-editor/src/context/defaultConfig.js
- packages/react-filerobot-image-editor/src/index.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~507-~507: Did you mean: “By default,”?
Context: ...: +v4.0.0 Default: false By default once the user makes any change/edit on ...
(BY_DEFAULT_COMMA)
🔇 Additional comments (1)
public/demo-config.js (1)
344-348:✅ Verification successful
Avoid property-name mismatches
You now expose two very similar flags:
// avoidChangesNotSavedAlertOnLeave // avoidChangesNotSavedAlertOnClose ← newly addedHaving 27-character identifiers that differ by only four letters is a typo-trap; a misspelled key will silently fall back to the default.
Please run a quick search to make sure every usage insrc/is spelled exactlyavoidChangesNotSavedAlertOnClose, and that comments for the Leave flag are still correct.
🏁 Script executed:
#!/bin/bash # List all occurrences of both flags to catch misspellings. rg -n $'avoidChangesNotSavedAlertOn(Leave|Close)'Length of output: 1650
No mismatched properties detected
A search for both
avoidChangesNotSavedAlertOnLeaveandavoidChangesNotSavedAlertOnCloseacross the repo shows they’re consistently spelled in:
- README.md
- public/demo-config.js
- packages/react-filerobot-image-editor/src/index.d.ts
- packages/react-filerobot-image-editor/src/context/defaultConfig.js
- ConfirmationModal.jsx
- App/index.jsx
The comment on the Leave flag in
public/demo-config.jsremains unchanged from before, so its semantics are still accurate.
Signed-off-by: Hamza <[email protected]>
hamza221
force-pushed
the
fix/discard-confirmation-modal-on-config
branch
from
1035cfe to
d8e63c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
500-509: Consistency & grammar: wrap property heading in backticks and add comma after "By default".The new property heading should match other entries (use backticks), and the phrase "By default" should be followed by a comma for clarity.
-#### avoidChangesNotSavedAlertOnClose +#### `avoidChangesNotSavedAlertOnClose` -By default once the user makes any change/edit on the image and hasn't saved the image yet and then clicks on the close button, a confirmation modal will be shown asking if they really want to close without saving. Setting this to `true` will skip showing the confirmation modal and close immediately. +By default, once the user makes any change/edit on the image and hasn't saved the image yet and then clicks on the close button, a confirmation modal will be shown asking if they really want to close without saving. Setting this to `true` will skip showing the confirmation modal and close immediately.🧰 Tools
🪛 LanguageTool
[uncategorized] ~507-~507: Did you mean: “By default,”?
Context: ...: +v5.0.0 Default:falseBy default once the user makes any change/edit on ...(BY_DEFAULT_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx(2 hunks)packages/react-filerobot-image-editor/src/context/defaultConfig.js(1 hunks)packages/react-filerobot-image-editor/src/index.d.ts(1 hunks)public/demo-config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react-filerobot-image-editor/src/index.d.ts
- public/demo-config.js
- packages/react-filerobot-image-editor/src/context/defaultConfig.js
- packages/react-filerobot-image-editor/src/components/Topbar/ConfirmationModal.jsx
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~507-~507: Did you mean: “By default,”?
Context: ...: +v5.0.0 Default: false By default once the user makes any change/edit on ...
(BY_DEFAULT_COMMA)
Summary by CodeRabbit
Summary by CodeRabbit