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 JBrowse R shiny app demo #3732

Merged
merged 8 commits into from Jun 7, 2023
Merged

Add JBrowse R shiny app demo #3732

merged 8 commits into from Jun 7, 2023

Conversation

carolinebridge
Copy link
Contributor

Adding a JBrowse R demo to our repertoire of live demos.

To be hosted on shinyapps.io.

@carolinebridge
Copy link
Contributor Author

carolinebridge commented May 30, 2023

  1. App currently copies the React demos, but this might not be totally desirable, e.g. 'see the state'...lemme know if that section is useful to users ;; also replaced "react to the view" with a section toggling tracks
  2. there's actually a few example apps already in the repo -- i actually did most of the work for this one before i found them, but what are our thoughts on a "big" app like this versus several small ones // versus the content available in those examples?
  3. the package itself might need some updating (we were talking about a version bump anyways):
  • since JBrowse was updated with smaller text, it's almost unreadable...thoughts on what might go into fixing this?
  • as far as i can tell, there's no output from the jbrowse r app -- that is, you can't detect on your shiny app what is happening inside the module, you can just manipulate it from the outside...this is an issue for things like external track togglers for example, might be something to add?

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label May 30, 2023
@carolinebridge carolinebridge added documentation and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels May 30, 2023
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Merging #3732 (a883e21) into main (68415a4) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3732      +/-   ##
==========================================
- Coverage   64.39%   64.39%   -0.01%     
==========================================
  Files         976      976              
  Lines       29592    29592              
  Branches     7094     7094              
==========================================
- Hits        19057    19055       -2     
- Misses      10370    10372       +2     
  Partials      165      165              

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator

are you able to deploy it to shinyapps.io to see it live? maybe in your personal account?

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 30, 2023

since JBrowse was updated with smaller text, it's almost unreadable...thoughts on what might go into fixing this?

not sure what version is deployed but the look and feel of the JBrowseR component "should" look the same, font size wise and such, as e.g. the @jbrowse/react-linear-genome-view e.g. in the storybook. the embedded components try to 'block out' external page styling that could affect font sizes, etc. possibly we need JBrowseR to get the updated @jbrowse/react-linear-genome-view if for some reason it is old enough that it doesn't have this

@carolinebridge
Copy link
Contributor Author

are you able to deploy it live

https://cbridge.shinyapps.io/jbrowse-r-shiny/

not sure what version

I think it's v2.0.1?

@cmdcolin
Copy link
Collaborator

gotcha. the "avoidParentStyle" class with the css selector all>initial is supposed to avoid font from the outer page bleeding into the inner page but something about the shiny page is bleeding through anyways

image

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 30, 2023

may be due to this particular entry in bootstrap css

html {
font-size: 10px;
}

untitled

The "initial" may refer to values in "html". you can reproduce similar effect in the embedded-demos and adding e.g. a <style>html { font-size: 10px; }</style> to the <head>

@cmdcolin
Copy link
Collaborator

note that the latest version of bootstrap may not add this 10px html style...putting e.g. <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bootstrap@4.0.0/dist/css/bootstrap.min.css" integrity="sha384-Gn5384xqQ1aoWXA+058RXPxPg6fy4IWvTNh0E263XmFcJlSAwiGgFAW/dAiS6JXm" crossorigin="anonymous"> in the <head> does not cause font size issues

@carolinebridge
Copy link
Contributor Author

using a bootstrap theme fixes it! thanks colin

@cmdcolin
Copy link
Collaborator

to be clear, i think the page already had some sort of bootstrap.min.css being loaded that added that weird 10px, but might be that the way you adjusted the page it fixed it. it is difficult to debug the issue anyways so this might be a good workaround

@cmdcolin
Copy link
Collaborator

here is a small way to reproduce the 'issue' without any bootstrap https://github.com/GMOD/jbrowse-components/compare/bad_style_html_bleed?expand=1&w=1

the all:initial may refer to the "html" section of css instead of some truly "initial" value, but i think you found a reasonable workaround

@cmdcolin
Copy link
Collaborator

looks good with the bootstrap addition you made :) 👍

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 2, 2023

what do you think about putting the code in embedded_demos? it may not be strictly the same as our other embedded demos but it may work out ok, and avoids a new top level folder

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 2, 2023

the other alternative is only keeping this code in the jbrowseR repo, but i am open to putting more jbrowseR stuff here. potentially the entire JBrowseR repo could even be moved into this monorepo...a multi-language monorepo. there are likely pros and cons to that

@carolinebridge
Copy link
Contributor Author

moved to existing folder and new functionality live on https://cbridge.shinyapps.io/jbrowse-r-shiny/

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 5, 2023

@carolinebridge-oicr can you run yarn format and fix spelling here https://github.com/GMOD/jbrowse-components/actions/runs/5180240247/jobs/9334165481?pr=3732#step:3:40

@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 5, 2023

may also need to merge main branch to fix https://github.com/GMOD/jbrowse-components/actions/runs/5180240032/jobs/9334164952?pr=3732#step:8:178 i think that was fixed on main

@carolinebridge carolinebridge marked this pull request as ready for review June 6, 2023 12:57
@cmdcolin
Copy link
Collaborator

cmdcolin commented Jun 7, 2023

will probably want to create a workflow to update this demo when we release a new version of jbrowseR. might also move it to gmod.shinyapps.io (I just registered this account), but overall, looks great!

@cmdcolin cmdcolin merged commit d3219d6 into main Jun 7, 2023
11 checks passed
@cmdcolin cmdcolin deleted the add-r-shiny-demo branch June 7, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants