search

att / rcloud

Collaborative data analysis and visualization
http://rcloud.social
MIT License
430 stars 142 forks source link

Bugfix/issue 2608 #2611

Closed jameesy closed 5 years ago

jameesy commented 5 years ago

Fixes #2608

Fix for the issue of inner scrolling in the editor panel by allowing the editor to grow depending on the amount of changes.

gordonwoodhull commented 5 years ago

Hmm, it looks like now the sections grow to the height of the containing div, but not necessarily far enough to show all of the changes.

For example, I'm replacing a long cell with another long cell here:

image

There are about 10 lines of the content to be deleted which are missing here, as well as another section which will be added.

I'm not sure, but I think that setting the height at 100% will use the height of the containing element, whereas we need to use the height of the content.

Actually, if I just remove max-height from .diff-panel then it seems to show everything - does this make sense?

jameesy commented 5 years ago

Strange. I can only assume for some reason maxHeight is being assigned as height in your circumstance. These are the variables that control the content. That wasn't my intention and it probably would have been the wiser option to remove it completely.

Please find changes below Gordon and advise if working correctly for yourself.

gordonwoodhull commented 5 years ago

Sorry, but that doesn't seem to help. Could you please try testing with some long diffs? If there is a very long cell (taller than the window) that is replaced with another very long cell, you should see the issue I am describing.

As I commented earlier, I think the problem is with the CSS - if you set max-height for .diff-panel then the diff-panel will not be allowed to be any taller than that height. If that value is 100%, that means 100% of the container, so we end up with the diffs for each cell being exactly as tall as that area of the dialog.

Setting it to a very large number like 1000px also seemed to help but it would be better if that restriction were not there at all.

jameesy commented 5 years ago

Max-height removed from the SASS file Gordon.

Irrelevant now, but funnily enough the previous commit worked fine in Firefox but not in Chrome. Lesson learned!

gordonwoodhull commented 5 years ago

Aha! I should have thought of that when we were seeing totally different things. I can confirm that the original commit works on Firefox but not Chrome.

Firefox is more standards-compliant, but I'm in the habit of testing on Chrome first because the developer tools are still so much better. (Even though I use Firefox as my main browser.)

Funny that the major browsers don't completely agree on the basic CSS Box model even in 2018. I think that shows how complicated this stuff is.

Rebased and merged for 2.1. Thanks @JABedford!