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-5937 "Save" button visible on bookmarks when logged out and on a tag's bookmarks page #4757

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

forceofcalm
Copy link
Contributor

Pull Request Checklist

Issue

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

Purpose

Wraps the "save" and "saved" actions for bookmarks in a logged_in? check.

Testing Instructions

I have left a comment for clarification on the JIRA ticket.

Credit

calm (they/them)

@ceithir
Copy link
Contributor

ceithir commented Apr 5, 2024

Could you add an integration test covering this fix?

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.

I think these should help with the ERB lint warning (I have not had a chance to test locally though, so some further nudges may be required)

app/views/bookmarks/_bookmarkable_blurb.html.erb Outdated Show resolved Hide resolved
app/views/bookmarks/_bookmarkable_blurb.html.erb Outdated Show resolved Hide resolved
@forceofcalm
Copy link
Contributor Author

I'm writing the integration test for this now (or at least, trying to), and I've noticed that the Save button is actually an <li> element styled like a button. For accessibility reasons, this should be an actual <button> element. A change like that is out of the scope of this ticket, but should be noted somewhere.

@forceofcalm
Copy link
Contributor Author

Hi! I have no fucking idea what RuboCop wants from me. I've tried a ton of different indents and it is still upset.

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.

If rubocop and/or erblint is giving me trouble, I usually will run

rubocop -A path/to/file.rb
# or
erblint -A path/to/file.html.erb

That will often do a lot more than we need, so I also use git add -p to selectively add lines to my commit, and then run git checkout path/to/file.rb to avoid changing a bunch of things at once. I sympathise with fighting erblint/rubocop though, so I've pushed what ERB lint seems to want to brianjaustin@6f6f854

features/bookmarks/bookmark_browse.feature Outdated Show resolved Hide resolved
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