-
Notifications
You must be signed in to change notification settings - Fork 519
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-6238 Random Items button on a collection doesn't work from the collection homepage #4462
Conversation
…on-doesnt-work-from-the-collection-homepage
Is there a way I can run the ERB lint runner locally? |
I don't typically run it locally, so I'm not sure how well it will match reviewdog (in theory it should be the same thing), but here's what you can try
if you are using docker compose
|
Could you add a cucumber test for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you internationalizing this file! There are few spots that have plain strings that would be nice to also get i18n ready. But no worries if you'd rather not bother with them.
<%= link_to "Random Items", url: collection_path(@collection, show_random: true), method: :get, remote: false %> | ||
<% else %> | ||
<%= link_to "Random Items", collection_path(@collection, show_random: true) %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this "Random Items" text translatable as well (by using t()
).
@@ -1,62 +1,60 @@ | |||
<div id="dashboard" class="region" role="navigation region"> | |||
<h3 class="landmark heading">Dashboard</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this text translatable as well, I would suggest t(".landmark.dashboard")
.
<% end %> | ||
</ul> | ||
|
||
<!-- challenge section of dash --> | ||
<% if @collection.challenge %> | ||
<%= render :partial => "challenge/#{challenge_class_name(@collection)}/challenge_sidebar" %> | ||
<%= render partial: "challenge/#{challenge_class_name(@collection)}/challenge_sidebar" %> | ||
<% end %> | ||
|
||
<h4 class="landmark heading">Contents</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this text translatable as well, I would suggest t(".landmark.contents")
.
|
||
<% if @collection.user_is_maintainer?(current_user)%> | ||
<% if @collection.user_is_maintainer?(current_user) %> | ||
<h4 class="landmark heading">Choices</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this text translatable as well, I would suggest t(".landmark.choices")
.
…on-doesnt-work-from-the-collection-homepage
Change in wrangling_guidelines is because I spotted a typo, and then the erb linter started complaining so I internationalized the file |
@@ -1,5 +1,5 @@ | |||
<!--Descriptive page name, messages and instructions--> | |||
<h2 class="heading"><%= link_to ts('Wrangling Guidelines'), wrangling_guidelines_path %> > <%= @wrangling_guideline.title %></h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this >
the typo you're referring to? It's not a typo -- it's part of the heading, to make it breadcrumb-like. For example, "Wrangling Guidelines > Wrangling Guidelines - Intro and General Concepts" on these guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, I'll fix that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much!
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-6238
Purpose
What does this PR do?
Testing Instructions
How can the Archive's QA team verify that this is working as you intended?
If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.
References
Are there other relevant issues/pull requests/mailing list discussions?
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.
Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.