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

CSS collisions between JBrowse and Tripal #777

Closed
enuggetry opened this Issue Jul 1, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@enuggetry
Copy link
Contributor

enuggetry commented Jul 1, 2016

@laceysander reports many CSS conflicts between JBrowse and Tripal/Drupal.
Thoughts: explore CSS namespace

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jul 26, 2016

I think CSS namespace refers to a sort of different type of thing. You can see it's kind of like totally global classes of CSS (xhtml tags or svg tags) http://stackoverflow.com/questions/10703291/is-anyone-actually-using-css-namespaces

So, maybe some other options might be worthwhile. Another idea is instead of adding the "tundra" class to document.body, it can be added added to the GenomeBrowser div only

Something like this change would contain all dijit tundra styles to the GenomeBrowser div

diff --git a/src/JBrowse/Browser.js b/src/JBrowse/Browser.js
index e3233d2..bc8aab5 100644
--- a/src/JBrowse/Browser.js
+++ b/src/JBrowse/Browser.js
@@ -436,7 +436,7 @@ fatalError: function( error ) {
             var container = this.container || document.body;
             var thisB = this;

-            dojo.addClass( document.body, this.config.theme || "tundra"); //< tundra dijit theme
+            dojo.addClass( container, this.config.theme || "tundra"); //< tundra dijit theme

             if( !Util.isElectron() ) {
                 require([
@@ -653,7 +653,7 @@ initView: function() {

         //set up top nav/overview pane and main GenomeView pane
         dojo.addClass( this.container, "jbrowse"); // browser container has an overall .jbrowse class
-        dojo.addClass( document.body, this.config.theme || "tundra"); //< tundra dijit theme
+        dojo.addClass( this.container, this.config.theme || "tundra"); //< tundra dijit theme

         var topPane = dojo.create( 'div',{ style: {overflow: 'hidden'}}, this.container );

I hypothesize that only applying the tundra class to this div would make the CSS more contained and reduce collisions. It seemed to look fine in the browser after doing this too. CC @laceysanderson

@laceysanderson

This comment has been minimized.

Copy link

laceysanderson commented Aug 2, 2016

From a CSS perspective the problems are actually two-fold: 1) Keeping JBrowse CSS contained, 2) Ensuring pre-existing page CSS does not adversely affect JBrowse.

I finally found time to create a JBrowse/Drupal concoction to demonstrate my problem. The following is a fresh install of JBrowse 1.12.1 with Volvox backbone and GFF loaded. I then created a number of Drupal pages using the page source from an existing site and copied over any CSS/JS needed. The following pages are available from the same JBrowse installation for debugging:

Obviously I changed the height/width on the container div to something more sensible than 100% and on the tripal-jbrowse-page.html I set the nav and tracklist to be hidden as you would expect it to be embedded on a gene page :-)

Using the developer tools it does look like Dojo is wreaking most of the havoc so thats definitely a good place to start.

Here are my files in a tarbomb so you can debug locally: http://knowpulse.usask.ca/jbrowse/embedded-JBrowse/drupalJbrowse.tar. Just extract that within an existing JBrowse dev environment and you should be ready to go. Thanks for working on this! :-)

@laceysanderson

This comment has been minimized.

Copy link

laceysanderson commented Aug 3, 2016

Follow up: I've made the two changes suggested by @cmdcolin above and the tundra class is still being added to the body. I added console.log() to both locations but neither appears to be called... I tried looking a bit deeper but immediately got lost ;-)

One thing I did notice is that there's a lot of document.body mentions throughout the code (ex: src/JBrowse/GenomeView.js) that should be checked to ensure it shouldn't actually be acting on the container rather than the whole page.

PS. JBrowse does some pretty aggressive caching. I use Chrome and cleared the cache "to the beginning of time" as well as used incognito so I don't think I'm seeing a cached version but... How do you normally ensure you're not encountering cached versions during development?

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Aug 3, 2016

I don't think the change that I mentioned really fixes it, and it also causes other issues. I wouldn't use it :)

I did find maybe an alternative approach with just prefixing all the styles from css/main.css with the jbrowse class

You can drop this file in your css/main.css to test it out
https://raw.githubusercontent.com/GMOD/jbrowse/d01d525df6877a4210cc96bd2941a9e325d3cdc8/css/main.css

Looks much better as far as layout

screenshot-localhost 2016-08-03 11-41-16-fs8

@laceysanderson

This comment has been minimized.

Copy link

laceysanderson commented Jan 18, 2017

Those changes do look worlds better! The Jbrowse is now actually functional :-) It looks like the main remaining issue is the tundra class still being applied to the body. For example, dojo is messing with the margin of the Lorem ipsum header and paragraphs in this page. Furthermore, it's changed the font of the entire page :-(
See http://knowpulse.usask.ca/jbrowse/embedded-JBrowse/drupal-jbrowse-page.html versus http://knowpulse.usask.ca/jbrowse/embedded-JBrowse/drupal-page.html

@nathandunn is excited about the progress here and asked if I would make it a pull request even though we're not quite done. Since this is your progress @cmdcolin would you like to make the pull request or should I and just make you the author of the commit?

PS. Sorry about the delay replying -I deleted my testing environment which took a while to find time to set back up again. I'll try to get the original tar I linked to back available ASAP to make this easier to test.

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jan 18, 2017

Very glad that this helps. i think the relevant branch is here https://github.com/GMOD/jbrowse/tree/css_isolation

I can do it if interested

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jan 18, 2017

I see from your demo it does still have glitches and it is expected from a couple of things but hopefully those can be fixed :)

@laceysanderson

This comment has been minimized.

Copy link

laceysanderson commented Jan 18, 2017

To be honest, it would likely be easier/faster for you to do it since I have yet to do any JBrowse development. I'm definitely interested in seeing this move forward and have had excited comments on the progress even though we're not quite there yet :-)

@cmdcolin

This comment has been minimized.

Copy link
Contributor

cmdcolin commented Jan 24, 2017

Added a pr for this

@laceysanderson

This comment has been minimized.

Copy link

laceysanderson commented Jan 24, 2017

Thanks @cmdcolin!!!

@rbuels rbuels added the feature req label Jan 30, 2018

@rbuels rbuels added this to the 1.14.0 milestone Apr 8, 2018

@rbuels rbuels closed this in d01567c Apr 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.