Skip to content
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.

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

AO3-5865 Reorganise 'Edit multiple works' form #4655

Merged
merged 18 commits into from
Apr 5, 2024

Conversation

denerose
Copy link
Contributor

@denerose denerose commented Nov 9, 2023

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-5865

Purpose

Reorganizes the 'Edit multiple works' form into correctly titled sections to better align with regular Edit and Post work forms.

Adds the 'verbose' class to the form (so section labels show). Updates capitalization of relevant labels within related sub-forms etc to sentence case where required to match recommendation from AO3-5865 but committed separately and can be rolled back if not desirable.

Testing Instructions

Review 'Edit multiple works' form. See: https://otwarchive.atlassian.net/browse/AO3-5865

References

N/A

Credit

Saphron (denerose) - she/her.

@denerose denerose marked this pull request as draft November 9, 2023 09:35
@denerose
Copy link
Contributor Author

Re: ERB Lint errors - I've left the trailing space on both blocks of checkboxes for readability (and also because it's not code I've actually otherwise touched). If this is not an acceptable exception please let me know and I'll update it to please the linter.

Re: cucumber tests: I have updated the relevant edit multiple tests to match the updated case/syntax on screen.

I cannot replicate the tag wrangling test fail on my side though, and can't find any reason for them based on the files touched. I am getting download and work share fails but also getting those on master so assuming this is not something I've caused or is an error on my end/setup issue. If I am causing this, please let me know where to look or what to roll back. Thank you!

@denerose denerose marked this pull request as ready for review November 11, 2023 00:44
@ceithir
Copy link
Contributor

ceithir commented Nov 11, 2023

Re: ERB Lint errors - I've left the trailing space on both blocks of checkboxes for readability (and also because it's not code I've actually otherwise touched). If this is not an acceptable exception please let me know and I'll update it to please the linter.

The wisest path is likely to abide by the linter's decision here, even if it might not be the best one in absolute. As one of the main goals of having a linter is to avoid thinking and debating too much about ultimately non critical things such as appropriate syntax.

I cannot replicate the tag wrangling test fail on my side though, and can't find any reason for them based on the files touched.

The "Grandparent" error appears to be unrelated to the work done here indeed, no need to worry about it. See there for ongoing investigation on that subject.

Copy link
Member

@brianjaustin brianjaustin left a comment

Choose a reason for hiding this comment

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

Hi Saphron! Thank you for the contribution!

IMO the readability isn't impacted too much with that, so I'd prefer we listen to the linter

Thanks again!

@denerose
Copy link
Contributor Author

denerose commented Nov 16, 2023

@brianjaustin thank you! The trick was to manually correct the blank lines in the checkbox blocks then run with --autocorrect. Just trying to autocorrect it all without doing that first had confused the linter too much and resulted in big far left aligned mess. It will look much prettier now (no pun intended) and linter happy.

I'll have a look at the i18n stuff on suggestion from @ceithir this weekend and see how I go before I re-push. Thanks to you both.

Copy link
Member

@brianjaustin brianjaustin left a comment

Choose a reason for hiding this comment

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

The feature test failure just look like capitalisation/wording changes. If you update

When I select "Coauthor's Work Skin" from "Select Work Skin"
that should fix things

app/views/works/_work_form_associations_language.html.erb Outdated Show resolved Hide resolved
Co-authored-by: Brian Austin <13002992+brianjaustin@users.noreply.github.com>
@denerose
Copy link
Contributor Author

The feature test failure just look like capitalisation/wording changes. If you update

When I select "Coauthor's Work Skin" from "Select Work Skin"

that should fix things

Thank you! Tests running now, fingers crossed.

<% end %>
<% end %>
</dd>
<% end %>

<dt><%= form.label :pseuds_to_add, ts("Add Co-Creators") %></dt>
<dt><%= form.label :pseuds_to_add, t("works.byline.add_co-creators") %></dt>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for updating translations.

Any particular reason some (like the ones five lines above) were left as is?

Seems good otherwise.

@brianjaustin
Copy link
Member

Hi @denerose! We're planning to include this in our next release, but it has some merge conflicts -- we can take care of them, but if you'd prefer to address them yourself, please let us know within ~48 hours. Thanks!

@brianjaustin brianjaustin merged commit bed484d into otwcode:master Apr 5, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants