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

Use new search box on reading log pages and author page #9563

Conversation

dwrik
Copy link
Contributor

@dwrik dwrik commented Jul 12, 2024

Closes #9557

This PR fixes issue #9557 by implementing new search box in author and reading log pages

Technical

Replaced input tags with render_template() call

Testing

Go to My Books -> Reading log (left side menu) pages or go directly to any author's page

Screenshot

Old search box:

Screenshot 2024-07-13 at 2 18 17 AM

New search box:

Author page
image

Reading log pages
image
image
image

Stakeholders

@cdrini

@cdrini
Copy link
Collaborator

cdrini commented Jul 15, 2024

Make the wording relative to the page it's on; e.g. "Search reading log" like it was before 👍

@cdrini cdrini assigned jimchamp and unassigned cdrini Jul 15, 2024
@dwrik
Copy link
Contributor Author

dwrik commented Jul 15, 2024

@cdrini Sure, will do. Thanks.

@dwrik dwrik force-pushed the 9557/fix/use-new-searchbox-on-readling-log-and-author-pages branch from 5b40169 to 5bf83e8 Compare July 16, 2024 14:57
@dwrik
Copy link
Contributor Author

dwrik commented Jul 16, 2024

@cdrini @jimchamp

Updated to reflect current page in placeholder text

Reading Log

image

Author Page

image

@dwrik
Copy link
Contributor Author

dwrik commented Jul 22, 2024

Hi @cdrini @jimchamp,

Just a friendly follow-up to see if there's any additional feedback or steps needed on this PR. If everything looks good, we can go ahead and merge it.

Thanks for your time!

Best,
dwrik

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @dwrik!
It looks like all of the placeholder strings are hard-coded English. The suggested changes, when made, will replace the English string with a translation in the patron's local language (if such a translation exists).

More information on how Open Library handles internationalization can be found here

openlibrary/templates/account/reading_log.html Outdated Show resolved Hide resolved
openlibrary/templates/search/searchbox.html Outdated Show resolved Hide resolved
openlibrary/templates/type/author/view.html Outdated Show resolved Hide resolved
@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 22, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jul 23, 2024
@dwrik dwrik force-pushed the 9557/fix/use-new-searchbox-on-readling-log-and-author-pages branch from e670eaa to dbc2286 Compare July 23, 2024 14:35
@dwrik
Copy link
Contributor Author

dwrik commented Jul 23, 2024

@jimchamp

Apologies, I missed the i18n translations. I've rebased the branch to include new changes and fixed the i18n issues by using the suggestions provided and fixed them where necessary (title was hardcoded, fixed that). Tested it locally and it works as expected. Let me know if any further changes required.

Thanks.

@dwrik dwrik requested a review from jimchamp July 23, 2024 14:43
@dwrik
Copy link
Contributor Author

dwrik commented Jul 29, 2024

@jimchamp,

I have addressed the requested changes. Just wanted to check if there are any additional steps needed from my side.

Thanks!

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Lgtm. Thanks, @dwrik!

@jimchamp jimchamp merged commit e3c8c5c into internetarchive:master Jul 29, 2024
4 checks passed
@LeadSongDog
Copy link

Not sure if this effort caused it, but:
IMG_1757

@cdrini
Copy link
Collaborator

cdrini commented Aug 13, 2024

^ Unrelated; will be fixed by #9719 .

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.

Use new search box on reading log pages, author page
4 participants