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-6238 Random Items button on a collection doesn't work from the collection homepage #4462

Conversation

Cesium-Ice
Copy link
Contributor

@Cesium-Ice Cesium-Ice commented Feb 24, 2023

Pull Request Checklist

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.

@Cesium-Ice Cesium-Ice marked this pull request as ready for review May 20, 2024 20:42
@Cesium-Ice
Copy link
Contributor Author

Is there a way I can run the ERB lint runner locally?

@brianjaustin
Copy link
Member

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

erblint app/views/<path/to/view>

if you are using docker compose

docker compose run test erblint app/views/admin/_admin_nav.html.erb

@Bilka2
Copy link
Contributor

Bilka2 commented May 23, 2024

Could you add a cucumber test for this?

Copy link
Contributor

@Bilka2 Bilka2 left a 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.

Comment on lines 35 to 37
<%= 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) %>
Copy link
Contributor

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>
Copy link
Contributor

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>
Copy link
Contributor

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>
Copy link
Contributor

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").

@Cesium-Ice
Copy link
Contributor Author

Change in wrangling_guidelines is because I spotted a typo, and then the erb linter started complaining so I internationalized the file

@Cesium-Ice Cesium-Ice requested a review from Bilka2 September 16, 2024 21:01
@@ -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>
Copy link
Collaborator

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.

Copy link
Contributor Author

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...

Copy link
Contributor

@Bilka2 Bilka2 left a 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!

@brianjaustin brianjaustin merged commit 36ca470 into otwcode:master Jan 22, 2025
29 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