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

Add a link to Archive from Advanced Search page #203

Merged
merged 2 commits into from
Mar 9, 2024

Conversation

redrun45
Copy link
Collaborator

@redrun45 redrun45 commented Mar 9, 2024

Another one with admin consensus: 👍

I tried combinations of things, but it doesn't look bad as just an h5. And there's no business logic to get wrong. This can go live at your convenience. 😃

I'm convinced there's some CSS thing I'm supposed to be adding, but matching the h5 up above... at least isn't adding to the mess?
@@ -101,6 +101,10 @@
</div>
</div>

<h5>Still can't find what you're looking for? Try searching <a rel="noopener" target="_blank" href="https://archive.org/details/librivoxaudio">Internet Archive's LibriVox Audiobook Collection</a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to dislike messing with the defaults for opening links, as it breaks expectations and removes control from the user (middle click to open in new tab only if I chose to), is there a reason you went with _blank? As far as I can tell, this isn't consistent with the other links on the advanced search page either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Our 'Internet Archive Page' links from the individual works' pages should probably be the model here.

Removed rel=noopener and target=_blank
@notartom notartom merged commit 7299225 into LibriVox:master Mar 9, 2024
1 check passed
@redrun45 redrun45 deleted the add-search-link branch March 9, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants