Repository navigation
Conversation
|
Assigning myself as reviewer on this since I have started, but it might be a few days because I want to poke at the form layout a bit! |
sarken
left a comment
There was a problem hiding this comment.
Thanks for realizing the form needed some context with this change! 😆 I looked at it a bit and I think I found a slightly simpler design pattern to use for it, even though it involves a little more work.
| And I view the work "Undecided" | ||
| And I follow "Edit" | ||
| And I follow "Orphan Work" |
There was a problem hiding this comment.
I think the three of these can be replaced with this step:
otwarchive/features/step_definitions/orphan_steps.rb
Lines 13 to 18 in b5f5328
| @@ -1,11 +1,11 @@ | |||
| When /^I choose to take my pseud off$/ do | |||
| step %{I choose "Take my pseud off as well"} | |||
| step %{I choose "Remove my pseud"} | |||
There was a problem hiding this comment.
How about updating these step definitions to use choose("use_default_false") and choose("use_default_true") while we're here? We do prefer the user-facing label text in the feature files themselves, but the IDs are fine and usually less brittle for the step definitions.
| <fieldset> | ||
| <legend><%= t(".prompt") %></legend> | ||
| <ul> | ||
| <li> | ||
| <%= radio_button_tag "use_default", "true" %> | ||
| <%= label_tag "use_default_true", t(".remove_pseud") %> | ||
| </li> | ||
| <li> | ||
| <%= radio_button_tag "use_default", "false" %> | ||
| <%= label_tag "use_default_false", t(".keep_pseud") %> | ||
| </li> | ||
| </ul> | ||
| </fieldset> |
There was a problem hiding this comment.
I think we can simplify this form considerably by just using this fieldset and no dl (and changing the .prompt key to .legend while we're there).
In order to make the legend visible, though, we'll need to add the classes single simple post to the form elements that use this partial.
Here's a preview of how the form should look once those changes are made:

Lastly, we should update the comment on the CSS for our single design pattern to say /* mods: SINGLE for prominent simple forms consisting of one text input (e.g., on the blocked user page) or group of checkboxes or radios (e.g., on the orphan work page) */
| it "rejects values other than true or false" do | ||
| post :create, params: { work_ids: [work], use_default: "maybe" } | ||
|
|
||
| it_redirects_to_with_error( | ||
| new_orphan_path(work_ids: [work.id]), error | ||
| ) | ||
| expect(work.reload.users).to include(user) | ||
| end |
There was a problem hiding this comment.
Since this is slightly different from providing no option, let's pull this out into its own context, like context "when invalid pseud option is provided".
Pull Request Checklist
AO3-1234 Fix thing)Issue
https://otwarchive.atlassian.net/browse/AO3-4052
Purpose
The orphan page preselected "Take my pseud off as well", so it was easy to orphan without reading the options. This PR:
OrphansController#createreject the form when no option (or an unexpected value) is submitted, with an error and a redirect back to the orphan page.legendfor screen readers, following the pattern used in the work form.The partial is shared by the work, series, pseud and account orphan pages, so all four are affected.
Testing Instructions
<pseud> (orphan_account)in the byline, unless the work is in an anonymous collection).orphan_account.Credit
Pablo Monfort (he/him)