Remove Bootstrap CSS and glyph Icon Set

[acthp]

Most of the components have been switched to react-toolbox, material ui css.

http://react-toolbox.io

A few components are still using bootstrap, including kmPlot.js, StateError.js, VizSettings.js. We need to refactor these to use react-toolbox widgets, and then remove the bootstrap css from the build (in main.js).

One potential tricky part is that some DOM elements are inadvertently being affected by both css libraries, so removing bootstrap css may introduce some unexpected layout flaws that will need to be fixed. In particular, both libraries define grid classes.

[syt123450]

Hi, I am working on this issue, and have already done refactor StateError.js using react-toolbox in my own repo. Now I want to test it in chrome to make sure it won't influence other component, could you show me how to trigger this component in chrome? Thanks.

[acthp]

Triggering it the real way is somewhat complicated, because you would have to coerce an error to happen by having invalid state.

You can fake it by setting stateError true in the initial state, in store.js, like

    var initialState = {
        version: 1,
        stateError: true,
        spreadsheet: {

With this change, each time you open a new tab it will display the warning until you dismiss it.

Just remember not to commit this change.

[syt123450]

@acthp Thank you for your guidance, I have tested my code in this way, and it works well in Chrome, Safari, and firefox. I send a PR for refactoring "StateError", if it is ok, I will continue refactoring other components.

[syt123450]

@acthp Hi, I am working on refactoring the kmPlot.js now. When I am refactoring the kmPlot.js using React-toolbox, I find this js files mix many features which can be split into several components to let them more readable and maintainable. My preliminary thoughts are split it into "kmPlot"(the modal body part), "kmPlot-utils"(like react-utils.js and utils.js, some util function for kmPlot), and "dialogContainer"(the pop up dialog container, which can be reused in other part of the project).

However, I am not sure whether it is proper for me to refactor the kmPlot in this way, looking forward for your suggestions.

Thanks.

[syt123450]

Hi, I am working on refactor VizSetting.js these days, how can I trigger this component in the UI for test?

[acthp]

@syt123450 Re: KmPlot.js, it's probably better to do the refactor for bootstrap separate from any refactor for modularity.

In this case, I don't think we want to break it down. We do often refactor to split math, or other data transform, from rendering. That makes it easier to test the math. Km is already structured that way, with the math in js/models/km.js, and the rendering in KmPlot.js. We also break down rendering if a file is getting large, or if the components need to be reusable or composable. In this case, all the parts are very specific to Km, so I'm not sure we'd gain much. A dialog container for the project might be interesting if we had many dialogs, but currently we don't. So for now I don't think it's worth doing.

Re: VizSettings, it's available on the column menu (button on upper right of column) under "Display". It isn't available for phenotype columns, or mutation columns. For gene expression, copy number, etc., it allows changing the color scales.

[syt123450]

@acthp Thank you for your suggestions. I have refactor the kmplot, in this refactoring, I only remove bootstrap dependencies without change the structure of kmplot.

I have send a pull request for this refactoring, if there is something need to be changed, I can modify it quickly.

If we would like to have a Dialog container in the future, I can handle it ~

[jingchunzhu]

@syt123450 we need a few additional styling changes on the km plot. 1) can you vertically align the warning icon with the text before it 2) can you add a bit spacing around the word "survival probability", or move it to the left of the chart and vertically aligned with the chart. Either will work for us, and you can decide. 3) can you remove the gray background on the close button?

[syt123450]

@jingchunzhu It's my pleasure to continue working on this issue, I will try my best to implement these feature tonight.

[syt123450]

@jingchunzhu I have fixed the issues you mentioned above, the kmPlot now looks as shown below:

The changes I made to kmPlot:

1. Align the button
2. Add left and bottom space to text "survival probability" in graph
3. Remove the background gray on the close button. However, I think the hover background color is important, so I have not remove it, and for P warning dialog, I have not remove the background color, do I need to remove it? If these background color need to be changed, I can immediately change them.

The adjust for these I have push to pull request #212

[jingchunzhu]

@syt123450

1. Align the button

excellent

2. Add left and bottom space to text "survival probability" in graph

I tested the code. I like it a bit further adjusted. Below, I added 5px padding on x and y, which looks good to my eyes.

transform='rotate(-90)'
y='5'
x={-height + 5}
dy='.71em'
textAnchor='start'>
Survival probability

3. Remove the background gray on the close button. However, I think the hover background color is important, so I have not remove it, and for P warning dialog, I have not remove the background color, do I need to remove it? If these background color need to be changed, I can immediately change them.

I like it. There is no need to change no. 3 further. Thanks for keeping the hover background.

4. A new item. The bottom of the KM plot page has a large bottom margin on my browser. And it gets larger when I zoom out on the browser. See below.

[syt123450]

@jingchunzhu I have added the two features your mentioned above, and push the code to the #212 , the change I did to kmPlot is:

1. Adjust the space of "survival probability"
2. Normalize the height of kmPlot dialog

The kmPlot now looks like this:

[jingchunzhu]

Adjust the space of "survival probability" Normalize the height of kmPlot dialog both look good. Thanks.

[syt123450]

@acthp I create two pull request related to this issue.

The first one is to fix stateError. When I refactor the stateError, I have not notice that the dialog can not overlay navbar and footer, I fix it in this PR #217 The second one is to refactor km.css into css module. This PR is not directly related to this issue, as I am familiar with km.css, I refactor it to modularize the css, I hope it will help to refactoring work. This PR is #218

Looking forward for your reviews and suggestions. Thanks.

[syt123450]

As I have removed bootstrap dependencies from StateError.js, KmPlot.js, VizSettings.js, I did some test removing bootstrap, I find that we still can not totally remove bootstrap for the reasons below:

1. Link style may become weird
2. Input widgets of chart view is influenced
3. Bootstrap dependencies exists in Column.js
4. "glyphicon" icon exists in project
5. SampleSearch.js has some Bootstrap widgets (but they are commented)

I will continue working on these issues.

However, I am not sure how to handle the widget in SampleSearch.js, do I need to refactor these code or directly delete them? Looking forward for your suggestions. @acthp

[maryjgoldman]

There is a bug where the styling on the 2, 3, quartile radio buttons in the KM plot is off. Right now the quartile radio button and text are on their own line below the 2- and 3- group radio buttons and text. But in the CC xena browser, the quartile radio button is on one line and the quartile text is on another (see yellow box in screenshot and http://cc.xenabrowser.net/heatmap/?bookmark=dfcb4c058fbe97181373d1e9f54b7647)

It should look like this instead:

[maryjgoldman]

There is also a bug in the CSS for the drop downs for the graph. Both in the positioning but also in the text padding.

TO FIX: I don't need it to look like bootstrap drop downs but I do need a) better positioning of each drop down so that they do not overlap and b) more text padding on the drop down options

Here is an example: http://cc.xenabrowser.net/heatmap/?bookmark=f964fab2cdf52384dd3325b77d0cf822

Bad one on CC:

Good one on production

[frano-m]

Awesome finding @maryjgoldman - I'll take a look today

[maryjgoldman]

Both bugs look good on the CC branch. Let me know if I should move this somewhere on the Zen Board

[maryjgoldman]

My last thing is that the PDF button for the KM plot doesn't work. Not sure if this is because it's dev build? I have a hard time imaging removing bootstrap is the reason why it is not working. @acthp

Otherwise, it looks great!

[maryjgoldman]

Nevermind! I can not get the no-KM-PDF bug to be triggered again. Must have been something transient on my side.