[59468] Danger Confirmation Dialog implementation #214

myabc · 2024-12-11T23:41:05Z

Based on FeedbackDialog. Wraps Primer::Alpha::Dialog, providing additional standardised behaviour for scenarios where a user performs a "dangerous" action.

Includes initial Lookbook previews and component tests.

Implements DangerConfirmationDialogFormHelperElement Catalyst controller providing checkbox confirmation behaviour.

What are you trying to accomplish?

Screenshots

https://raw.githubusercontent.com/opf/primer_view_components/d232bfc77ee23bcde7e00ac3bd50c70bc2f4d034/.playwright/screenshots/snapshots.test.ts-snapshots/primer/open_project/danger_confirmation_dialog/default/light.png

Integration

List the issues that this change affects.

https://community.openproject.org/projects/openproject/work_packages/58680

Risk Assessment

Low risk the change is small, highly observable, and easily rolled back.
Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Fixes axe scan violation - This change fixes an existing axe scan violation.
No new axe scan violation - This change does not introduce any new axe scan violations.
New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

Additional TODOs/Questions for Discussion

Determine how to handle I18n Resolution: leave to caller
~~Test with Rails form builders (?)~~ Resolution: test once upstream

changeset-bot · 2024-12-11T23:41:08Z

🦋 Changeset detected

Latest commit: 43e1243

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@openproject/primer-view-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-12-11T23:48:49Z

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

HDinger

Hi @myabc

nice job, it looks really good 👍 I only have some minor remarks.

app/components/primer/open_project/danger_confirmation_dialog.html.erb

app/components/primer/open_project/danger_confirmation_dialog.rb

previews/primer/open_project/danger_confirmation_dialog_preview.rb

previews/primer/open_project/danger_confirmation_dialog_preview/playground.html.erb

myabc · 2024-12-16T21:34:17Z

@HDinger I've addressed most of the issues you raised.

In addition to the couple of questions inline, a couple general questions:

Should we use checkbox/Checkbox or check_box/CheckBox? The HTML spec uses checkbox, and is what I would normally prefer. It's one less keystroke for our end users. However the latter would make us consistent with the other Primer components.
DangerConfirmationDialog is a pretty long, cumbersome name IMO. "confirmation" is probably redundant, given that our slots also our named confirmation_message and confirmation_dialog. Could we consider going with just (an opinionated) DangerDialog?

app/components/primer/open_project/danger_confirmation_dialog.html.erb

myabc · 2024-12-16T21:52:09Z

app/components/primer/open_project/danger_confirmation_dialog/confirmation_checkbox.rb

+
+        def call
+          render(Primer::BaseComponent.new(**@system_arguments)) do
+            render(Primer::Alpha::CheckBox.new(**@checkbox_arguments.merge(label: trimmed_content)))


@HDinger As I've implemented it, we build the checkbox label based on (trimmed_)content - this allows for a similar API to the other slots, i.e.:

dialog.with_show_button { "Click me" } dialog.with_confirmation_checkbox { "I confirm this deletion" }

An alternative would be to add a label_text string parameter - either on DangerConfirmationDialog itself - or on the slot. I am not sure "what feels" more idiomatic?

DangerConfirmationDialog.new(label_text: "I confirm this deletion") dialog.with_confirmation_checkbox(label_text: "I confirm this deletion")

Also, do we want to define a I18n.t default for the label? Or is it fine to always leave it to the caller?

Currently you can write something like dialog.with_confirmation_checkbox which will render a checkbox without any label. So we either have to provide a default or enforce that the user provides a string. Since the default is rather special, we probably have to enforce the the user to pass an argument for now.

With regards to the syntax, I like the dialog.with_confirmation_checkbox { "I confirm this deletion" } notation, but if it is easier to enfoce a text with a named parameter, I am also fine with that.

Based on `FeedbackDialog`. Wraps `Primer::Alpha::Dialog`, providing additional standardised behaviour for scenarios where a user performs a "dangerous" action. Includes initial Lookbook previews and component tests. Implements `DangerConfirmationDialogFormHelperElement` Catalyst controller providing checkbox confirmation behaviour. https://community.openproject.org/projects/openproject/work_packages/58680

Adds `confirmation_checkbox` slot to allow the caller to provide custom labels, including I18n strings.

HDinger

I left some remarks and will check what is happening with the snapshots.

.changeset/brave-eggs-obey.md

test/components/component_test.rb

HDinger · 2024-12-17T07:24:27Z

app/components/primer/open_project/danger_confirmation_dialog/confirmation_checkbox.rb

+
+        def call
+          render(Primer::BaseComponent.new(**@system_arguments)) do
+            render(Primer::Alpha::CheckBox.new(**@checkbox_arguments.merge(label: trimmed_content)))


Currently you can write something like dialog.with_confirmation_checkbox which will render a checkbox without any label. So we either have to provide a default or enforce that the user provides a string. Since the default is rather special, we probably have to enforce the the user to pass an argument for now.

With regards to the syntax, I like the dialog.with_confirmation_checkbox { "I confirm this deletion" } notation, but if it is easier to enfoce a text with a named parameter, I am also fine with that.

app/components/primer/open_project/danger_confirmation_dialog.html.erb

Ensure the the form is actually submittable.

Makes the capitalisation of CheckBox/check_box consistent with other Primer components. Renames the internal `ConfirmationCheckBox` component and slots (`with_confirmation_check_box`) accordinly.

* Teaches `form_arguments` `:builder` and `:name` options. `:builder` should be an instance of `ActionView::Helpers::FormBuilder`, which is created by the standard Rails `#form_with` and `#form_for` helpers. * Uses Rails Form Helpers under-the-hood to render form tags, ensuring automatic generation of CSRF token, `_method` hidden inputs.

HDinger

Nice job 🌟 I really like the way it can be used 👍

myabc force-pushed the feature/58680-danger-confirmation-dialog branch from 843ae27 to 0fb1cb5 Compare December 12, 2024 00:07

HDinger requested changes Dec 12, 2024

View reviewed changes

myabc force-pushed the feature/58680-danger-confirmation-dialog branch 4 times, most recently from e38d4bb to 0ab3d92 Compare December 16, 2024 20:53

myabc requested a review from HDinger December 16, 2024 21:29

myabc commented Dec 16, 2024

View reviewed changes

app/components/primer/open_project/danger_confirmation_dialog.html.erb Show resolved Hide resolved

myabc commented Dec 16, 2024

View reviewed changes

myabc force-pushed the feature/58680-danger-confirmation-dialog branch from 161029a to f3848d1 Compare December 16, 2024 21:59

myabc added 6 commits December 16, 2024 19:07

Flesh out Danger Confirmation Dialog system test

da9aa2a

Remove Loading State param from Playground Preview

973401e

Allow confirmation checkbox label customisation

97c0789

Adds `confirmation_checkbox` slot to allow the caller to provide custom labels, including I18n strings.

Validate confirmation_{message,checkbox} provided

0cede74

Remove broken inline styling in Playground Preview

d60a8e6

myabc force-pushed the feature/58680-danger-confirmation-dialog branch from f3848d1 to d60a8e6 Compare December 16, 2024 22:07

Generating component snapshots

867d87b

github-actions bot added the patch release label Dec 16, 2024

HDinger requested changes Dec 17, 2024

View reviewed changes

Create changeset

0d265ad

myabc force-pushed the feature/58680-danger-confirmation-dialog branch from 4e56bad to 0d265ad Compare December 17, 2024 11:30

github-actions bot added minor release and removed patch release labels Dec 17, 2024

myabc added 3 commits December 19, 2024 07:25

Render button[type='submit'] when form is rendered

aa84913

Ensure the the form is actually submittable.

Make capitalisation of CheckBox consistent

d605b29

Makes the capitalisation of CheckBox/check_box consistent with other Primer components. Renames the internal `ConfirmationCheckBox` component and slots (`with_confirmation_check_box`) accordinly.

Add system test to test form submission

59fef6e

myabc added 2 commits December 19, 2024 22:35

Validate content block passed to ConfirmationCheckBox

43e1243

myabc force-pushed the feature/58680-danger-confirmation-dialog branch from f12553a to 43e1243 Compare December 20, 2024 01:38

myabc requested a review from HDinger December 20, 2024 01:42

HDinger approved these changes Dec 20, 2024

View reviewed changes

HDinger merged commit 4a8aee9 into main Dec 20, 2024
33 checks passed

HDinger deleted the feature/58680-danger-confirmation-dialog branch December 20, 2024 07:42

openprojectci mentioned this pull request Dec 20, 2024

Release Tracking #216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[59468] Danger Confirmation Dialog implementation #214

[59468] Danger Confirmation Dialog implementation #214

myabc commented Dec 11, 2024 •

edited

Loading

changeset-bot bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

HDinger left a comment

myabc commented Dec 16, 2024

myabc Dec 16, 2024

myabc Dec 16, 2024

HDinger Dec 17, 2024

HDinger left a comment

HDinger Dec 17, 2024

HDinger left a comment

[59468] Danger Confirmation Dialog implementation #214

[59468] Danger Confirmation Dialog implementation #214

Conversation

myabc commented Dec 11, 2024 • edited Loading

What are you trying to accomplish?

Screenshots

Integration

List the issues that this change affects.

Risk Assessment

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Merge checklist

changeset-bot bot commented Dec 11, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 11, 2024 • edited Loading

⚠️ Visual differences found

HDinger left a comment

Choose a reason for hiding this comment

myabc commented Dec 16, 2024

myabc Dec 16, 2024

Choose a reason for hiding this comment

myabc Dec 16, 2024

Choose a reason for hiding this comment

HDinger Dec 17, 2024

Choose a reason for hiding this comment

HDinger left a comment

Choose a reason for hiding this comment

HDinger Dec 17, 2024

Choose a reason for hiding this comment

HDinger left a comment

Choose a reason for hiding this comment

myabc commented Dec 11, 2024 •

edited

Loading

changeset-bot bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading