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

[sql lab] Make sql editor resizable #3242

Merged
merged 18 commits into from
Aug 19, 2017
Merged

[sql lab] Make sql editor resizable #3242

merged 18 commits into from
Aug 19, 2017

Conversation

dmigo
Copy link
Contributor

@dmigo dmigo commented Aug 4, 2017

Implements #2237

The idea of this PR is to be able to resize the editor of the sql lab.

@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage remained the same at 69.34% when pulling 8c9a8311f5b4909e6fcbd8bffe8b6df917ec8c2f on dmigo:master into 4c3313b on apache:master.

@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage remained the same at 69.34% when pulling a4079c3391b884badb32886371fb9f3ad1007e67 on dmigo:master into 4c3313b on apache:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.34% when pulling 78c00e3fbfcb15c1269f845c2e8be9d4341b5e88 on dmigo:master into 4c3313b on apache:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage remained the same at 69.34% when pulling 78c00e3fbfcb15c1269f845c2e8be9d4341b5e88 on dmigo:master into 4c3313b on apache:master.

@mistercrunch
Copy link
Member

Glad to see you're working on this. There was some issues in SQL Lab around resizing the window where the height of some components wouldn't be altered as they should. Please make sure resizing the window work well along with this set of changes.

@dmigo
Copy link
Contributor Author

dmigo commented Aug 4, 2017

@mistercrunch I encountered 2 problems with resizing while working on this PR.
First: it wasn't possible to resize react-ace properly it had some issues. This one is solved by updating to the version 5.1.2.
Second: Sql Lab has one hack in it. This hack adjusts sizes of some parts of the page. I didn't fix it, since it is out of scope of the PR, however I have a couple of ideas about it.

Update: tried to resize the browser's window in different ways - seems to work.

@dmigo dmigo changed the title WIP: Make sql editor resizable Make sql editor resizable Aug 4, 2017
@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage increased (+0.2%) to 69.311% when pulling ab0e0165e7e51cde05f5ee5dcf8a7d34ce2084f8 on dmigo:master into ef7e9dd on apache:master.

@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage remained the same at 69.311% when pulling ab0e0165e7e51cde05f5ee5dcf8a7d34ce2084f8 on dmigo:master into ef7e9dd on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.311% when pulling ab0e0165e7e51cde05f5ee5dcf8a7d34ce2084f8 on dmigo:master into ef7e9dd on apache:master.

@coveralls
Copy link

coveralls commented Aug 4, 2017

Coverage Status

Coverage remained the same at 69.311% when pulling 6f07444157cb45e7bce5148f6a89400979672b7e on dmigo:master into ef7e9dd on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.311% when pulling 6f07444157cb45e7bce5148f6a89400979672b7e on dmigo:master into ef7e9dd on apache:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.311% when pulling 6f07444157cb45e7bce5148f6a89400979672b7e on dmigo:master into ef7e9dd on apache:master.

@mistercrunch
Copy link
Member

I pulled the branch and tried it. It mostly works but it's a little funky. We went from single-page app to no-quite that where there's one too many scrollbars on the lower right panel.

I don't think the lower-right panel should have a fixed height anymore. Meaning if/when you scroll down to look at the data in the result pane the SQL Editor would flow out of the screen, leaving more place for to browse the results.

@mistercrunch
Copy link
Member

I'd also like to have a minimum height constraint on the SQL editor, so that users can't get confused as to it completely disappearing.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no scroll on the lower right and minimum height on SQL editor

@dmigo
Copy link
Contributor Author

dmigo commented Aug 7, 2017

@mistercrunch got it 👍

@dmigo dmigo changed the title Make sql editor resizable WIP: Make sql editor resizable Aug 7, 2017
@dmigo
Copy link
Contributor Author

dmigo commented Aug 8, 2017

@mistercrunch
It is going to be hard with

no scroll on the lower right

On the lower right many tabs have react-virtualized which doesn't support scroller on the nesting element out of the box.
We could create our own component for that, or try to extend react-virtualized.
Either way it seems to be a separate big task.

Another one possible solution would be to have one common scroller for the page. Instead of two separate scrollers for the two columns we have.

We could also go the jsfiddle way. it is simply not possible to extend anything there so, that some part of the page goes beyond the screen.

@coveralls
Copy link

coveralls commented Aug 8, 2017

Coverage Status

Coverage remained the same at 69.311% when pulling b91e95a on dmigo:master into 0429e84 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.311% when pulling b91e95a on dmigo:master into 0429e84 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 69.234% when pulling 1b3430f on dmigo:master into 0429e84 on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 69.234% when pulling 1b3430f on dmigo:master into 0429e84 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 69.234% when pulling 1b3430f on dmigo:master into 0429e84 on apache:master.

@mistercrunch
Copy link
Member

Right, sorry I didn't think that through. Then it should probably just be a full-page app, same as jsfiddle works for me.

@dmigo
Copy link
Contributor Author

dmigo commented Aug 15, 2017

@mistercrunch
That wasn't easy, but currently this PR seems to consider all that there is on the page.
The functionality is ready for a usability test. However I would like to put some more work into this PR. I'll list the todos in the main commit.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.388% when pulling 90d0dc8 on dmigo:master into e31ad22 on apache:master.

1 similar comment
@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage remained the same at 69.388% when pulling 90d0dc8 on dmigo:master into e31ad22 on apache:master.

@dmigo
Copy link
Contributor Author

dmigo commented Aug 18, 2017

@mistercrunch it is ready in terms of code. Should I squash all the changes into one commit?

@dmigo dmigo changed the title WIP: Make sql editor resizable [sql lab] Make sql editor resizable Aug 18, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.388% when pulling 20bbb07 on dmigo:master into e31ad22 on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.388% when pulling 20bbb07 on dmigo:master into e31ad22 on apache:master.

@coveralls
Copy link

coveralls commented Aug 18, 2017

Coverage Status

Coverage remained the same at 69.388% when pulling 20bbb07 on dmigo:master into e31ad22 on apache:master.

@mistercrunch
Copy link
Member

Not need to squash, we use the Github "Squash and merge" button. Let me try it out!

@mistercrunch
Copy link
Member

img

@mistercrunch mistercrunch merged commit 75e69f0 into apache:master Aug 19, 2017
@mistercrunch
Copy link
Member

@dmigo found a bug related to this, when popping a new tab things don't init right:
screen shot 2017-08-23 at 9 31 45 am

mistercrunch added a commit to mistercrunch/superset that referenced this pull request Aug 23, 2017
mistercrunch added a commit that referenced this pull request Aug 23, 2017
@dmigo
Copy link
Contributor Author

dmigo commented Aug 29, 2017

@mistercrunch sorry, didn't notice the comment in time. Did #3363 solved all the problems?

@mistercrunch
Copy link
Member

Everything is good now, leveraging react-split-panes abstracted a lot of the complexities

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.19.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants