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 7 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 and others added 2 commits April 20, 2024 11:01
Co-authored-by: Brian Austin <13002992+brianjaustin@users.noreply.github.com>
Co-authored-by: Brian Austin <13002992+brianjaustin@users.noreply.github.com>
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants