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

SCC-3897 - Search Results Electronic Resources Link #55

Merged
merged 9 commits into from
Dec 12, 2023

Conversation

dgcohen
Copy link
Contributor

@dgcohen dgcohen commented Dec 8, 2023

Ticket: https://jira.nypl.org/browse/SCC-3897

This PR adds the ElectronicResourcesLink component that is mostly derived from the ElectronicResourcesResultsItem component in DFE, with a few minor adjustments.

@dgcohen dgcohen requested a review from EdwinGuzman December 8, 2023 16:45
Copy link

vercel bot commented Dec 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
research-catalog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2023 5:25pm

{bib.materialType && <Text>{bib.materialType}</Text>}
{bib.publicationStatement && <Text>{bib.publicationStatement}</Text>}
{bib.yearPublished && <Text>{bib.yearPublished}</Text>}
<Text>{bib.itemMessage}</Text>
</Box>
{bib.electronicResources && (
Copy link
Member

Choose a reason for hiding this comment

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

In the Vercel preview, all bib search results are displaying the ElectronicResourcesLink and that's not what true for the same searches in production. Something seems to be off.

Copy link
Contributor

@charmingduchess charmingduchess Dec 11, 2023

Choose a reason for hiding this comment

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

I think the issue can be addressed here

this.electronicResources = result.electronicResources || null

There should be a check for electronicResources?.length because an empty array is truthy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a helper function hasElectronicResources that checks for the length. This resolves the issue.

</DSLink>
) : (
<RCLink href={`${bibUrl}#electronic-resources`}>
<Text
Copy link
Member

Choose a reason for hiding this comment

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

Don't render p tags inside links.

In this case, I think this block can be a Link with DS type of "standalone" https://nypl.github.io/nypl-design-system/reservoir/v2/?path=/story/components-navigation-link--with-controls&args=type:standalone
This will render the arrow for you.

Copy link
Member

@EdwinGuzman EdwinGuzman left a comment

Choose a reason for hiding this comment

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

One minor update but the logic check for when to display needs a closer look.

Copy link
Contributor

@charmingduchess charmingduchess left a comment

Choose a reason for hiding this comment

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

Looks good, except for the bug that has resources rendering for all bibs. I have one small ask- can you change the name of this PR to be ER or Electronic Resources Link instead of EDD?

@dgcohen dgcohen changed the title SCC-3897 - Search Results EDD Link SCC-3897 - Search Results Electronic Resources Link Dec 11, 2023
@dgcohen dgcohen merged commit 5764ef6 into main Dec 12, 2023
3 checks passed
@dgcohen dgcohen deleted the SCC-3897/search-results-edd branch December 18, 2023 17:20
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