Bioconductor / BiocStyle

Issues and pull requests for BiocStyle should go here.
12 stars 19 forks source link

BiocStyle update for mobile devices #67

Closed grimbough closed 4 years ago

grimbough commented 4 years ago

Currently BiocStyle doesn't render particularly nicely on narrow screens e.g. if you're reading a vignette on a phone screen #59. The left and right margins dominate and compress the main content to be almost unreadably narrow e.g.

The pull request add some additional CSS that comes into play when the screen width is less than 768px. The changes include:

Examples of some of these can be seen below:

For 'medium' sized screen (768px <= width < 992px) like a tablet, it picks a hybrid of the existing and narrow styles, with the left-hand floating navigation retained, but most of the other changes still implemented e.g.

I've tested this on a few different vignettes and it looks like it's working on those, but I'm happy to iterate on both style and substance before this is fully incorporated.

aoles commented 4 years ago

Looks great, thank you @grimbough for your contribution! I will be happy to review the PR within the next few days.

Cheers, Andrzej

grimbough commented 4 years ago

Hi @aoles just wondering if you'd had a chance to look at this yet. I'm happy to make some changes if needed.

aoles commented 4 years ago

Hi @grimbough,

Thanks for the reminder - I will get back to this ASAP.

Cheers!

grimbough commented 4 years ago

Hi @aoles any chance you'll get to take a look at this before the next release in April? I'm happy to do some more work if there's things you don't like, but I think it'd be cool too improve the small screen support for BioC 3.12.

aoles commented 4 years ago

Hi @grimbough,

let me apologize for the delay, this went a little bit out of my radar. I will prioritize this from now on and get back to you soon.

Cheers, Andrzej

grimbough commented 4 years ago

No worries Andrzej. I've been putting off fixing the BiocWorkflowTools error for almost as long!

grimbough commented 4 years ago

Hi Andrzej,

Thanks for the feedback. I think I've addressed the horizontal scrollbar issue. It now looks like this for me when I narrow the window:

image

The scrollbar only appears when needed, but it will do that rather than wrap the contents of code blocks.

aoles commented 4 years ago

Looks good to me, thanks a lot for your update!

I've cleaned up some formatting in https://github.com/grimbough/BiocStyle/pull/1. Once this is resolved I'm happy to continue with the merge.

Cheers, Andrzej

aoles commented 4 years ago

Thanks again Mike for the improvements to the style!

lcolladotor commented 4 years ago

Ohh nice, thanks for doing this!

For your set of edge cases, at http://bioconductor.org/packages/release/bioc/vignettes/brainflowprobes/inst/doc/brainflowprobes-vignette.html we used knitr::include_graphics() and out.width = 850 (code here) to include some PDFs like shown in the screenshot below.

Screen Shot 2020-04-23 at 9 43 43 PM

I'm curious to see how this will play out with the new version on small screens. We might have to update the out.width to be <= 768.

Best, Leo

grimbough commented 4 years ago

Thanks @lcolladotor. This is live in BiocDevel, so you can check out how things look already.

Those don't seem to show up on my Android phone or when I switch the browser to emulate and iPhone. I just get a big white space. This space is 850px wide, and so there's now a horizontal scroll bar for the whole page.

Screenshot from 2020-04-24 16-58-34

I don't think this problem was introduced by out changes though, as I also don't see the images in the vignette for the release version of the package. Is there a specific reason why you're going with PDFs embedded in HTML, rather than a more compatible image format?

lcolladotor commented 4 years ago

Thanks, I just checked on my laptop browser (Google Chrome) and it does show up in both release and devel like this:

Screen Shot 2020-04-25 at 10 14 35 AM

Then on my phone (iPhone) using Google Chrome I can also see the PDFs. So it's all working (as in displayed) for brainflowprobes. IMG_9824

I should have checked before mentioning it :P As to why this package uses PDFs instead of other images, that's how the author wrote it (it's their first package). Anyway, since it works, I won't suggest changing it. Though hm... you just said it doesn't work for you on your Android phone. Hm...