final fixes/tweaks for beta contract #2

[eplebel]

• [x] ARTICLE CARD esthetic tweaks:

  • author/year text better green: #006621
  • entire badge row opacity: .55 (maintaining pre-existing opacities for badges and commentaries labels)
  • negative margin on more button div (to reduce extra white space wasting vertical space)
  • moreButton color: lightgrey #d3d3d3
  • move article type and RR labels to the top, cleaned up a bit:
  • add horizontal line separator (lightgrey)
  • remove label borders for both article type and RR labels
  • update entire div to have opacity:.4, but then darken back to opacity:1 on card hover

• [ ] exposing 1st key figure on article card: small swipeable carousel of the 1st key figure (if available) in bottom-right corner of article card (which when clicked, opens media viewer as usual; same carousel as on landing page).

  • if not even 1 image available, then nothing displayed/no carousel;
  • the figure list in the expanded part of the card stays the same
  • if image available and all 3 full-text links exist, hide the preprint link div to leave more vertical space for carousel image (if only PDF and preprint links available, preprint link is NOT hidden)
  • opacity of .6 for article card carousel image, which changes to 1.0 on article card hover (same hover that displays the impact metrics; NOTE there's already code that darkens everything slightly on card hover)

• [x] add "/" searchbox keyboard shortcut that activates focus inside searchbox (on all pages; just like Google/Gmail, GitHub

  • [x] also SHIFT+ "/" to activate focus inside filter box on the AUTHOR PAGE article list

• [x] add 2 exceptions for the full-screen mode PDF embedded viewer (given they don't work, so don't want to show full-screen icon):

  • researchGate .pdf links, i.e., anything with "researchgate.net", example: https://www.researchgate.net/profile/Robert_Calin-Jageman/publication/298898208_Direct_and_Conceptual_Replications_of_Eskine_2013_Organic_Food_Exposure_Has_Little_to_No_Effect_on_Moral_Judgments_and_Prosocial_Behavior/links/5a027e8daca2720df3cbddb7/Direct-and-Conceptual-Replications-of-Eskine-2013-Organic-Food-Exposure-Has-Little-to-No-Effect-on-Moral-Judgments-and-Prosocial-Behavior.pdf
  • any pdf URL with "core.ac.uk", example: https://core.ac.uk/download/pdf/82008503.pdf

• [ ] embedded author page article list media viewer height issue (image is vertically centered across entire height of iframe instead of height of current viewport; which you're already aware of; but see screenshot as a refresher) UPDATE FEB 13, 2020: given you fixed additional involved issue (move article card actions to a menu), we can punt on this for now!

• [x] on mobile screen size (360px, eg Samsung A5), 2 viewport width, alignment issues:

  • viewport issue when expanding cards: for an article card with only 1 to 3 key figures, card expands fine; but if expand card w/ 4 or more figures, then article card width becomes incorrect (cut-off on the right); (UPDATE: probably just need to set the figure list to overflow-x right? In the future, we could get fancier and make it a horizontally-touch-scrollable image list (like on gMaps))
  • media viewer alignment issue: when figure is displayed in media viewer, horizontal alignment is off (cut-off on the right); this is true regardless of the # of figures (1, or >2, etc.) UPDATE (Feb 1): updated some facts & re-wrote/re-worded the explanation to clarify the issues; the 2 issues could be related, or could be separate issues...

remaining from tweaks to tweaks requests:

• [x] for mobile screen (<375px), hide "Added date" for Recent page article cards

• [x] embedded author article list from staging is not loading (not sure when it's stopped working but i suspect probably related to the full-screen mode updates

  • pulling author article list from PRODUCTION still works: https://etiennelebel.com/author/etienne-p-lebel-EMBED-AUTHOR-PAGE-from-staging.html using
  • pulling author article list from STAGING, DOES NOT work: https://etiennelebel.com/author/etienne-p-lebel-EMBED-AUTHOR-PAGE-from-staging.html using

    Uncaught Error: Component must be wrapped with <Container.Provider>
    at Object.useContainer (unstated-next.mjs:21)
    at ViewEmbeddedContentButton (ViewEmbeddedContentButton.jsx:77)

[eplebel]

wow article card already looks a lot more beautiful with https://github.com/ScienceCommons/curate_science/compare/0a33ac82e5...8b3aa664cb!; a few tweaks to the tweaks:

• tweak positioning of article type and RR labels (see screenshot below):
  • vertically align within header: i achieved it by adding margin-top:2px & margin-bottom:-2px)
  • slight left-margin alignment so article type text lines up with article title: looks like -3px
• update article type and RR labels darker color ON HOVER to opacity: .7 (instead of 1)
• moreButton needs to be closer to card edge (not further; perhaps i could have better explained this): looks like updating margin-bottom (from 5px) to -14px to the .ArticleContent-moreIconHolder-1445 class might do the trick (see screenshot below)
• reduce the size of all Transparency badges (from 30px x 30px) to 25px x 25px
• for Recent page article card, move "Added date" text to the header, right-aligned (& vertically aligned like article type & RR labels) and update text color to lighter grey (#DCDCDC) (see screenshot below)
• for mobile screen (<375px), hide ALL impact metrics (for all article cards) and hide "Added date" for Recent page article cards

oh and another thing:

• embedded author article list from staging is not loading (not sure when it's stopped working but i suspect probably related to the full-screen mode updates
  • pulling author article list from PRODUCTION still works: https://etiennelebel.com/author/etienne-p-lebel-EMBED-AUTHOR-PAGE-from-staging.html using
  • pulling author article list from STAGING, DOES NOT work: https://etiennelebel.com/author/etienne-p-lebel-EMBED-AUTHOR-PAGE-from-staging.html using

    Uncaught Error: Component must be wrapped with <Container.Provider>
    at Object.useContainer (unstated-next.mjs:21)
    at ViewEmbeddedContentButton (ViewEmbeddedContentButton.jsx:77)

[eplebel]

wow, all of these tweaks (https://github.com/ScienceCommons/curate_science/compare/8a0e064b66...117395d751) are perfect, looking really scrumptious!

at the fear of appearing insufficiently humble, you do realize @mickaobrien that you've built/implemented one of the most deliciously beautiful web products (UIs) in the entire academic science space!!? (and it's just the beginning/tip of the ice berg!!)

[eplebel]

oh, just noticed, one issue left out is: Hide "Added date" for Recent page article cards for mobile screen (<375px), which i've added to the list above (https://github.com/ScienceCommons/curate_science/issues/94#issue-554260660), in addition to the displaying embedded author page article list issue

[mickaobrien]

@eplebel Haha! Glad to be a part of it! :tada:

Yup, hiding the "Added date" text is next on my list, didn't get a chance to do it yesterday. Hopefully should be able to sort that and the author embed tomorrow.

[mickaobrien]

@eplebel Do you want the smaller figure carousel to stay visible when the article card is expanded and the full figure list is visible?

[eplebel]

actually it would be ideal to hide the smaller figure carousel on article card expand, as long as when the card is collapsed, that the smaller figure carousel is then displayed again.

[mickaobrien]

@eplebel You were right about the the Action buttons on the right causing the issue with the media viewer being cut off on mobile screens. I've started work on moving the actions to a dropdown menu on the article card (as in #95). It's a bit involved (everything effects everything!) but I should have it done next week.

[eplebel]

ok sounds good

[eplebel]

one last small request for a minor issue i just noticed that is crucial for demos:

article cards in embedded author page article lists are clipped on the left & right edges (looks like applying padding:16px to outer div class="AuthorPage-root-1" does the trick).

that said, further investigation revealed that this issue is also present on non-embedded article lists, but the issue only appears when the div container >550px AND in multi-panel context. in non-multi-panel (normal) mode, sufficient padding is achieved via AppContent horizontal padding (padding:0 1rem (16px), see screenshot below) WHEREAS this is not the case in a multi-panel mode, where the correct padding is somehow only being applied up to a max-width of 1100px (550px being 50% of 1100px). so i guess a better fix should address this issue AND the padding issue in an embedded AP article list context! (though i could be off here...)

[mickaobrien]

@eplebel I just pushed a fix for the padding issue in 18b80a30bd94dd47ee2479123f9e03588c08c797. I think it solves all the issues (the padding on the article list when the embedded viewer is visible or the screen is small and the padding on the embedded article list). Let me know if it's not working as expected.

[eplebel]

awesome, yup https://github.com/ScienceCommons/curate_science/commit/18b80a30bd94dd47ee2479123f9e03588c08c797 seems to have fixed the padding issue in both spots, thanks for quick fix!

and re: exposed carousel thumbnails, just WOW. this really takes it to the next level, especially that it's also deliciously pleasant on small screens!

everything is working, i just noticed a few final aesthetic issues.

final requests:

• [ ] thumbnails should always be a square with constant height x width of 80px x 80px (just like the current figure thumbnails list in the expanded card), even on mobile (though we can optimize this later)
• [ ] add padding-left:4px to exposed thumbnail so there's at least a tiny bit of space between the thumbnail and any text appearing to the left of it
• [ ] reduce the width of the carousel arrows (i.e., .carousel .control-next.control.arrow) so that there's more room to click on/swipe thumbnail , i.e., from 28px-wide to 14px (reducing padding:5px to 0px and margin: 0 5px to 0 3px seems to do the trick, but there might be a better solution)

requests (though i can live w/out these if not quick fixes):

• [ ] exposed thumbnail should be aligned to the bottom-right of the article card (though if this is not an easy fix i can live with the current positioning; if so, please reduce the top-margin from .5rem to .125 (or margin-top 8px to 2px) to save vertical space (when article content div is shorter than full-text links + exposed thumbnail)
• [ ] is some kind of "lazy loading" easy to implement for exposed thumbnails? (to speed up the author page loading time when there are lots of articles w/ thumbnails)? (if not i can live w/ this)

thanks again for your Herculean effort on this!!!

[mickaobrien]

@eplebel I just pushed some updates there:

• set size to 80x80px
• reduced the width of the arrows
• reduced the top margin (there's a bit of work needed to tidy up the layout to make it properly aligned too the bottom right so hopefully this works for now)

Do you have an example of the text being squeezed up against the viewer (where you want to add padding-left: 4px)? I can't see it and just want to make sure the changes I make do the job.

I think the lazy loading thing would be tricky but I'll have a look to see if there are quick solutions.

[eplebel]

ok awesome! i'm seeing the reduced arrow width and top margin updates, but i'm not seeing the 80x80px update. any ideas? (the build looks like it went through ok)

[mickaobrien]

Ah, there was some code missing. I've just pushed an update (f7372e625df764516558ba5f3c7e47667406418b), hopefully it looks ok now.

[eplebel]

I think the lazy loading thing would be tricky but I'll have a look to see if there are quick solutions.

ok sounds good.

• example of margin-left issue: go to https://curate-science-staging-2.appspot.com/app/author/etienne-p-lebel, for Galaxy S5 viewport (360px), and scroll down to "Registered Replication Report (RRR) of Finkel..." .25rem (4px) of space between viewer and text seems sufficient (see screenshot below)

• the carousel arrows are better, but i noticed there's now a minor margin issue whereby the arrows are not flush to the left and right edges of the thumbnail (see screenshot above). updating .MiniFigureViewer .carousel .control-arrow margin to margin: 0 0px (instead of 0 3px) seems to do the trick (and leaving .MiniFigureViewer .carousel .control-arrow::before margin: 0 3px). but again, perhaps there is a more sound/elegant solution.

• the only other thing i noticed is that extra vertical space is present on article cards when NO figure is available (see screenshot), which seems to be caused by displaying an empty miniFigureViewer div

ok so close! after these minor tweaks, i'll do some final testing, and then we can push everything to production!!

[eplebel]

Ah, there was some code missing. I've just pushed an update (f7372e6), hopefully it looks ok now.

yup i can see it now, thx!

[mickaobrien]

@eplebel I just pushed a few more commits that should address all the issues above. And I think I misunderstood what you meant about bottom aligning the MiniFigureViewer, it's actually pretty straightforward and commit 0425c1ea1c8b329fe88dcfe82bdfd379ec9f6ec0 should do the job (if my understanding now is correct!). Should it be like this?

Let me know if I've missed anything!

[eplebel]

ok, looks like you've covered everything! and yes, that's precisely what i meant with bottom aligning the MiniFigureViewer!

the only tiny thing i noticed is: The new "column" layout for article card (since exposed thumbnail) is preventing authors/pub year and journal name TEXT from overflowing toward the right edge of the card, hence creating lots of extra white space when impact metrics are available. So if you could just update the viewport min-width for hiding impact metrics to 540px (from 375px) , then i think we'll be all good. (in the future, we can better optimize this, by trimming long author lists, and even trimming long titles....)

though i still need to finish all regression base and UI tests before we push to production, but i'll let you know!

oh and a quick question: What is the best strategy to speed up author page loading performance: Reducing the # of articles w/ at least 1 exposed thumbnails AND/OR reducing the total # of figures?

[mickaobrien]

@eplebel Just pushed a fix to hide the impact metrics at 540px here: 197176cebbcb43bc9f64efe68d5633bf5ad0e45e

I'll have a look at the Author Page loading, I think it might be possible to add lazy loading to the carousel but I need to have a look.

[eplebel]

awesome, thx. and sounds great re: lazy loading!

[mickaobrien]

@eplebel Just pushed a version with lazy-loading there (e7451fb824e213bb9795f3f71e36d192de8e4f10). I think it should work but want to have a look around on staging first.

But the build is failing with this error:

Your app may not have more than 210 versions. Please delete one of the existing versions before trying to create a new version.

I've had a look through the Google Cloud Platform console but I think I have access to the master deployment (curate-science) not the staging (curate-science-staging). Not sure what the best way to fix it is, maybe @alexkyllo would know?

[eplebel]

ok @mickaobrien i've now granted you Editor, App Engine Admin, and Error reporting admin on the curate-science-staging-2 project in GCP. please let me know if you need access to anything else.

manually deleting old deployments is probably best solution for now, but looks like there might be better automated solutions to this (according to this but i of course have no idea). @alexkyllo any ideas?

[mickaobrien]

@eplebel Thanks for that! I deleted a bunch of old versions manually, there's probably a better way of handling this in general but it's cleared some space for the moment...

I've pushed the lazy loading now — it loads the first image automatically, the next and previous image when you hover over the first image and any subsequent images when they're one image away (i.e. next or previous in the carousel). This has reduced the number of image requests on page load e.g. on your author page, before lazy loading there were 90 image requests on page load, now there are 43. One outstanding issue is that a lot of the images are much bigger than they need to be. I see the code around generating thumbnails is there but I don't think it's saving the file to the database (even though the thumbnail is being created) and those thumbnails aren't being used anywhere on the frontend. I know there are plans to increase the size of the thumbnails on the article page (#64) and maybe other changes so it might be worth looking at as part of these changes.

[alexkyllo]

I just spent some time looking into this app version thing and it doesn't appear to be very well documented. Travis CI has a note here: https://docs.travis-ci.com/user/deployment/google-app-engine/#version-to-deploy

Version to deploy

Either the version flag or the default option must be set. 
If default is true, the default version will be deployed to, which will be
http://your-project-id.appspot.com. If the version flag is set instead,
it will deploy to http://version-dot-your-project-id.appspot.com.

Right now this flag is not set in our .travis.yml, but my guess is that the default behavior is equivalent to setting version: default. We can try setting it to a named version and see if subsequent deployments overwrite that version instead of creating more versions.

[eplebel]

@eplebel Thanks for that! I deleted a bunch of old versions manually, there's probably a better way of handling this in general but it's cleared some space for the moment...

I've pushed the lazy loading now — it loads the first image automatically, the next and previous image when you hover over the first image and any subsequent images when they're one image away (i.e. next or previous in the carousel). This has reduced the number of image requests on page load e.g. on your author page, before lazy loading there were 90 image requests on page load, now there are 43. One outstanding issue is that a lot of the images are much bigger than they need to be. I see the code around generating thumbnails is there but I don't think it's saving the file to the database (even though the thumbnail is being created) and those thumbnails aren't being used anywhere on the frontend. I know there are plans to increase the size of the thumbnails on the article page (#64) and maybe other changes so it might be worth looking at as part of these changes.

wow, this is really next-level stuff! huge kudos. the lazy loading updates have indeed improved page load performance noticeably, i'm very happy with it. and the pre-loading on HOVER is really slick. i left a comment on the relevant commit (https://github.com/ScienceCommons/curate_science/commit/3a50744cafccd22bde7f3ccca235efca1a0b5b4f#commitcomment-37459220) re: a small issue for touch-enabled devices.

And that's very interesting re: thumbnails, which I was completely unaware of (so thx so much for this info...). Yes it's good enough for now, but definitely should be further optimized in the future!

[eplebel]

I just spent some time looking into this app version thing and it doesn't appear to be very well documented. Travis CI has a note here: https://docs.travis-ci.com/user/deployment/google-app-engine/#version-to-deploy

Version to deploy

Either the version flag or the default option must be set. 
If default is true, the default version will be deployed to, which will be
http://your-project-id.appspot.com. If the version flag is set instead,
it will deploy to http://version-dot-your-project-id.appspot.com.

Right now this flag is not set in our .travis.yml, but my guess is that the default behavior is equivalent to setting version: default. We can try setting it to a named version and see if subsequent deployments overwrite that version instead of creating more versions.

Thanks so much Alex for looking into this. Your solution sounds good, but wouldn't that mean that we wouldn't have previous builds to revert back to? Or I guess that could be handled within TravisCI and/or gitHub? If so, then sure you can go ahead and update the .yml file for both prod and staging, thx!