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 pub details to pub card, and various css fixes #438

Merged
merged 7 commits into from
Aug 3, 2021
Merged

Conversation

vincerubinetti
Copy link
Contributor

@vincerubinetti vincerubinetti commented Jul 30, 2021

related: #420

  • fixes footer css on small screens
  • makes side nav bar fixed width for better readability, and collapses it sooner (as page width decreases) to avoid text wrapping clutter
  • rewrites getPublication to accept list of ids instead of a single id, and refactors
  • other minor css changes

still in progress

@netlify
Copy link

netlify bot commented Jul 30, 2021

✔️ Deploy Preview for monarch-ui ready!

🔨 Explore the source changes: 1ea554a

🔍 Inspect the deploy log: https://app.netlify.com/sites/monarch-ui/deploys/61081e825229ba00088127be

😎 Browse the preview: https://deploy-preview-438--monarch-ui.netlify.app/

Copy link
Collaborator

@falquaddoomi falquaddoomi 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 to me!

@vincerubinetti
Copy link
Contributor Author

@kshefchek and/or @jmcmurry can you take a look at what I have so far (which addresses the first bullet point of #420). I didn't add publisher because the table was already getting too crowded, so I thought title, author, and year would be enough identifying information at a glance. And you can always click on the publication to get more authors and details.

@kshefchek
Copy link
Contributor

perhaps instead of showing the PMID id we replace that with the title? I think that's how it was originally intended but we ended up using PMIDs as a fall back (and as you can see we haven't stored many titles in our db)

@vincerubinetti
Copy link
Contributor Author

perhaps instead of showing the PMID id we replace that with the title? I think that's how it was originally intended but we ended up using PMIDs as a fall back (and as you can see we haven't stored many titles in our db)

This is implemented now.

@jmcmurry
Copy link
Member

This looks amazing, thanks! My only feedback is that it would be great if possible to include the journal. To make room, we can jettison date specifics (Year is fine).

@vincerubinetti
Copy link
Contributor Author

This looks amazing, thanks! My only feedback is that it would be great if possible to include the journal. To make room, we can jettison date specifics (Year is fine).

Incorporated. I used the field source instead of fulljournalname, as source appears to consistently be an (accepted/standard?) abbreviation of the full journal name.

@vincerubinetti
Copy link
Contributor Author

@jmcmurry Can you review the update and approve

Copy link
Member

@jmcmurry jmcmurry left a comment

Choose a reason for hiding this comment

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

Amazing! Thanks

@jmcmurry jmcmurry merged commit 8cf8dbb into master Aug 3, 2021
@vincerubinetti vincerubinetti deleted the pub-details branch August 3, 2021 16:48
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.

4 participants