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 Full Text search links for PMC in HTML Assembler #1120

Merged
merged 7 commits into from Jul 10, 2020
Merged

Add Full Text search links for PMC in HTML Assembler #1120

merged 7 commits into from Jul 10, 2020

Conversation

PritiShaw
Copy link
Contributor

Adds Scroll To Text Fragment feature for links to PMC journals, when

  • add_full_text_search_link is True in make_model()
  • PMCID is available for the sentence

The links will automatically search for the sentence, scroll to it, and highlight, without using any plugin.
Highlighted Sentence

Sample Output

HTML Assembler output, source
Script used to generate above output
Sample Output from HTML Assembler

Supported Browsers

  • Chrome v80+for both desktop and android
  • Chromium-based Edge, Opera

@bgyori
Copy link
Member

bgyori commented Jul 6, 2020

Thanks a lot @PritiShaw, looks good! I have a question about this condition:

if not set(['PMCID', 'pmcid']) \
    .issubset(evidence["text_refs"].keys()):

It looks like here you have the {'PMCID', 'pmcid'} set and compare this against evidence["text_refs"].keys() which will typically be something like dict_keys(['PMID', 'TRID', 'PMCID', 'DOI', 'TCID', ...]). So there will never be an issubset relation and the if condition will thus always be satisfied. So this condition should be replaced by

if 'PMCID' not in evidence.get('text_refs', {})

There are a few other obscure corner cases that we'll fix (like PMID missing, etc.) once you made this change. Thanks!

@PritiShaw
Copy link
Contributor Author

if 'PMCID' not in evidence.get('text_refs', {})

Thanks for your review, I have modified the if the condition. I tested the code with the same script (output)
I was trying to take care the condition where PMID is missing, but could not understand what will be the fallback to get PMCID.

Thanks

@dianakolusheva dianakolusheva self-requested a review July 10, 2020 16:33
Copy link
Contributor

@dianakolusheva dianakolusheva 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, I just added a few changes related to ID lookups and formatting.

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.

None yet

3 participants