Skip to content

Support MessageSpec.extraConfirmText for high-impact confirm prompts #4064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Aug 13, 2025
Merged

Support MessageSpec.extraConfirmText for high-impact confirm prompts #4064

merged 10 commits into from
Aug 13, 2025

Conversation

jskupsik
Copy link
Contributor

@jskupsik jskupsik commented Aug 7, 2025 •
edited
Loading

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

@jskupsik
Copy link
Contributor Author

jskupsik commented Aug 7, 2025

Toolbox PR: xh/toolbox#776

@jskupsik jskupsik changed the title Added confirmation text to various DELETE statements Added confirmation typing for several delete actions Aug 7, 2025
@jskupsik jskupsik mentioned this pull request Aug 7, 2025
Closed
@lbwexler
Copy link
Member

lbwexler commented Aug 8, 2025

@jskupsik -- I love that you are doing this and had been thinking this recently -- in fact I have a PR for another change to our pop-up infrastructure:

#4054

This seems to me an elegant solution with the current API, but I am wondering if it would make sense to take it one step farther, to have MessageSpec include a 'extraConfirmText' and an extraConfirmLabel. Advantages would be:

  • Would be easy to document on MessageSpec, (instead of a hidden utility function)
  • Could be specified in addition to an input. (not sure how common this would be).
  • Could appear in a different place/color/size than the input, if our base app container wanted to make it look different than a standard prompt input.

@jskupsik
Copy link
Contributor Author

  • Enhanced the CHANGELOG message to better fit our style.
  • Added extraConfirmText and extraConfirmLabel properties to MessageSpec.
  • Implemented as additional extraConfirm field on the message's formModel, so it can be combined with input if so desired.
  • Changed all new usages of it to follow the new format.

@jskupsik jskupsik requested a review from lbwexler August 11, 2025 15:45
@jskupsik jskupsik changed the title Added confirmation typing for several delete actions Added extraConfirm typing prompt to MessageSpec, for several admin delete actions Aug 11, 2025
@lbwexler
Copy link
Member

🔥 -- Really like this change

lbwexler
lbwexler reviewed Aug 12, 2025
View reviewed changes
lbwexler
lbwexler reviewed Aug 12, 2025
View reviewed changes
amcclain
amcclain reviewed Aug 13, 2025
View reviewed changes
Copy link
Member

@amcclain amcclain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this on! You will see I recommend scaling back our use of this in Hoist, but the role delete and instance shutdown cases are good ones, and having this feature available for app usage will certainly be helpful.

admin/tabs/general/alertBanner/AlertBannerModel.ts Outdated
@@ -136,6 +136,7 @@ export class AlertBannerModel extends HoistModel {
removePreset(preset: PlainObject) {
XH.confirm({
message: 'Are you sure you wish to delete this preset?',
extraConfirmText: 'DELETE',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, no extra confirm - let's save it for truly high-impact choices that could destabilize a system if done (and then confirmed) accidentally. Deleting a banner preset not one of those cases.

desktop/appcontainer/Message.ts Outdated
})
})
: null
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit might be more readable with conditional checks and pushing onto items vs. multiple ternaries, or use omit which is designed for this

@amcclain amcclain changed the title Added extraConfirm typing prompt to MessageSpec, for several admin delete actions Support MessageSpec.extraConfirmText for high-impact confirm prompts Aug 13, 2025
@jskupsik
Copy link
Contributor Author

jskupsik commented Aug 13, 2025 •
edited
Loading

Validated that it's working on desktop and mobile.
image

image

amcclain
amcclain reviewed Aug 13, 2025
View reviewed changes
@amcclain
Tweaks during code review
c14259a 
- Don't inline label for confirm - might be long
- Tweaks to confirm language across toolkit usages for consistency
amcclain
amcclain approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@amcclain amcclain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@lbwexler lbwexler merged commit 63ca68d into develop Aug 13, 2025
2 checks passed
@lbwexler lbwexler deleted the requireDeleteConfirmTyping branch August 13, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lbwexler lbwexler lbwexler left review comments

@amcclain amcclain amcclain approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jskupsik @lbwexler @amcclain