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

author page infobox #8949

Closed
wants to merge 10 commits into from
Closed

author page infobox #8949

wants to merge 10 commits into from

Conversation

RayBB
Copy link
Collaborator

@RayBB RayBB commented Mar 23, 2024

CLOSED IN FAVOR OF #9130

This contributes to the Wikidata project #8236

This is on testing now: https://testing.openlibrary.org/authors/OL345358A/

It creates an infobox for the author page that includes:

  • Author name
  • Photo
  • Birth
  • Death
  • "Date" (if neither of the above are available)
    • There are 23005 records with this field. More info on what they are here.

Why?

  • For the Wikidata project we'd like to have an infobox with lots of the new data from Wikidata
  • We don't want patrons to have an inconsistent experience where sometimes there is an infobox and sometimes there isn't.
  • As such, we are making this first PR to get an infobox showing on all pages.

Technical

  • This design is heavily inspired from the Wikipedia infobox. Example
  • I am absolutely not tied to the colors/sizes/spacing/ect and would love someone to suggest improvements

Screenshot

After - using the style of the search results

Dates

image

No Dates

image
Before image
Old Mockups after - using wikipedia style

Dates

image

No Dates (doesn't look as nice)

image

Stakeholders

@mekarpeles

@RayBB RayBB added Needs: Design Feedback Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 1 Do this week, receiving emails, time sensitive, . [managed] labels Mar 23, 2024
@mekarpeles mekarpeles added Priority: 2 Important, as time permits. [managed] and removed Priority: 1 Do this week, receiving emails, time sensitive, . [managed] labels Apr 9, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Nice looks good! I think longer term we might want to have a discussion about how to structure this page; eg the subjects I think do make sense above the list of books on mobile (similar to our books pages with collapsing), but considering out of scope for now, since this is just a stepping stone for some of the wikidata changes we wish to make. As such I'm mainly considering whether this will have negative consequences for any existing workflows. So let's get some librarian eyes since they use birth/death date a lot for deduping authors.

I do think we should move the Author name from above the photo to below and adjust spacing as needed to make things balanced. Otherwise lgtm!

@RayBB
Copy link
Collaborator Author

RayBB commented Apr 15, 2024

@cdrini I've move the name under the image and fixed the spacing as we discussed.

I also posted in the librarian channel here to gather comments.

@RayBB
Copy link
Collaborator Author

RayBB commented Apr 16, 2024

@cdrini all the changes discussed and approved on the call today are here!

  • Remove name under image
  • Show date in both places for librarians

Try it out: https://testing.openlibrary.org/authors/OL1394244A/

We are ready to merge 🚀

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Apr 16, 2024
@RayBB
Copy link
Collaborator Author

RayBB commented Apr 24, 2024

@cdrini please let me know if there is anything else I need to do here. As far as I know all feedback has been incorporated.

@RayBB
Copy link
Collaborator Author

RayBB commented May 1, 2024

CLOSED IN FAVOR OF #9130

@RayBB RayBB closed this May 1, 2024
@RayBB RayBB removed Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 2 Important, as time permits. [managed] Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels May 1, 2024
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.

3 participants