AO3-7211 Add a warning to the success message when directly adding existing bookmark to collection #5481
Conversation
|
Hi, germpy! Thank you so much for this pull request. Someone will be along to review it soon. I've updated the Jira issue status to In Review to make sure no one mistakenly creates a duplicate pull request. If you'd like the ability to comment on, assign, and transition issues in the future, you're welcome to create a Jira account! It makes things a bit easier for us on the organizational side if the Full Name on your Jira account either closely matches the name you'd like us to credit in the release notes or includes it in parentheses, e.g. "Nickname (CREDIT NAME)." Once you've done that (or if you've already done it -- Jira has been unreliable about showing us new accounts in the admin panel lately), you can either reply here or send an email to otw-coders@transformativeworks.org with both your account name and email address and we'll set up the permissions for you. Thanks again for contributing! If you have any questions, you can contact us at the same email address listed above. |
|
An email has been sent, thank you! |
omerfaruk-pseud
left a comment
omerfaruk-pseud
left a comment
There was a problem hiding this comment.
Thank you so much for this PR! Two small problems, but looks pretty good.
| unless new_collections.empty? | ||
| flash[:notice] = ts("Added to collection(s): %{collections}.", | ||
| collections: new_collections.collect(&:title).join(", ")) | ||
| flash[:notice] += t("bookmarks.create.warnings.private_bookmark_added_to_collection") |
There was a problem hiding this comment.
Per our internationalization standards, you should avoid reusing the exact same locale key across different contexts, and use lazy lookup. Can you change this locale key to a new one and add it to locale file?
| unless new_collections.empty? | ||
| flash[:notice] = ts("Added to collection(s): %{collections}.", | ||
| collections: new_collections.collect(&:title).join(", ")) | ||
| flash[:notice] += t("bookmarks.create.warnings.private_bookmark_added_to_collection") |
There was a problem hiding this comment.
And also, in this situation the note wouldn't be added to endings of invited and unapproved collections' success messages. Can you move this line to a bit lower, just before where the message is created?
There was a problem hiding this comment.
Yep! Just to clarify, should I move it to replace the empty message in line 137 (which is only assigned if new_collections and unapproved_collections are empty), or after the unless statements at around line 155?
There was a problem hiding this comment.
The latter, line 155, (though what line 137 does is kinda the opposite of what you said.)
Bilka2
changed the title
4 participants
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7211
Purpose
What does this PR do?
Builds on AO3-7205, but adds the warning when directly adding existing bookmark to a collection with the Add to Collection button. This PR adds that warning to the success message when directly adding existing bookmark to collection.
Testing Instructions
How can the Archive's QA team verify that this is working as you intended?
Build UI locally, follow repro steps in Jira ticket AO3-7211. Video of local testing included.
testing.collection.bookmarks.mp4
References
AO3-7205 Add a warning to the bookmark success message
Credit
Credit under "germpy" is fine, she/her. I dunno if this is a change that really requires credit tbhhhh