AO3-7404 Enhancements to works New Comment page#5743
AO3-7404 Enhancements to works New Comment page#5743bre-nana wants to merge 15 commits intootwcode:masterfrom
Conversation
|
Sorry about the reviewdog lint failure - I can't seem to figure it out 😅 |
| <%= render partial: "comments/comment_form", locals: { | ||
| comment: @comment, | ||
| commentable: @commentable, | ||
| button_name: t(".actions.comment") | ||
| } %> |
There was a problem hiding this comment.
"the start of the line" here is the ruby code excluding the <%= tag, so you need to indent lines 8-11 by 4 more spaces
erblint actually has the -a option for autocorrecting errors which is especially nice for small files like this one (or where there aren't errors in code irrelevant to your pr in general), here the command would be erblint -a app/views/comments/new.html.erb (or docker compose run --rm web bundle exec erblint -a app/views/comments/new.html.erb if you're using docker)
| page_heading_html: Reading Comments on %{link} | ||
| new: | ||
| actions: | ||
| comment: Comment | ||
| page_heading_html: New Comment on %{link} |
There was a problem hiding this comment.
link variables should be descriptive, i suggest commentable_link like in the strings below
| <!--subnav--> | ||
| <p class="navigation actions" role="navigation"><!--TODO: INVESTIGATE this baffling link--><%= link_to ts("Back"), @commentable.is_a?(Tag) ? tag_path(@commentable) : polymorphic_path(@commentable) %></p> | ||
| <!--/subnav--> |
There was a problem hiding this comment.
the subnav comments could be removed too if you want to, i'm pretty sure there's no reason for them to be here anymore
| Scenario: Reading comments page for a work displays correctly | ||
|
|
||
| Given the work "Generic Work" | ||
| When I am logged in as "commenter" | ||
| And I visit the comments page for the work "Generic Work" | ||
| Then I should see "Reading Comments on" | ||
| And I should see a page link to the work "Generic Work" within ".comments-index h2.heading" | ||
|
|
||
| Scenario: New comment page for a work displays correctly | ||
|
|
||
| Given the work "Generic Work" |
There was a problem hiding this comment.
also just a nitpick since i'm already requesting other changes, but the empty lines after scenario descriptions aren't needed, so you can remove them in these two tests
| title = link_to(commentable.commentable_name, commentable) | ||
| end | ||
| (ts('Reading Comments on ') + title).html_safe | ||
| t("comments.index.page_heading_html", commentable_link: title) |
There was a problem hiding this comment.
ah, also sorry for not catching this earlier, looks like comments helper i18n strings should go into comments_helper in the config/locales/helpers/en.yml file
There was a problem hiding this comment.
ahh yep thanks! still trying to wrap my head around how the config/locales folder is structured 😅
should be fixed on adef561
slavalamp
left a comment
slavalamp
left a comment
There was a problem hiding this comment.
thanks! this looks good to me, it needs to be approved by an official volunteer now (assuming i didn't miss anything else)
3 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-7404
Purpose
On the New Comment page of a work:
Also added some translations for the Comments page header, and the "Comment" button on the New Comment page.
Testing Instructions
Below is an additional testing scenario to ensure the changes I've made on the Comments page haven't changed the functionality. I plan to comment this on the Jira ticket when/if this PR is merged.
Steps to reproduce
/works/xxx/comments)What happens (and what should still happen)
The page header says "Reading Comments on [work title]" and the work title is a link to the work.
Credit
bre-nana (she/they/he)