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

fix: Remove gap from SQLLab results bottom #19138

Merged

Conversation

codemaster08240328
Copy link
Contributor

SUMMARY

The table height was being calculated manually inside code, but it was based on the calculation in PREVIEW table.

In the RESULT tab, we need some correction to remove the gaps.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

image

AFTER:

image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@@ -152,7 +152,7 @@ export default function SouthPane({
query={latestQuery}
actions={actions}
user={user}
height={innerTabContentHeight}
height={innerTabContentHeight + 24}
Copy link
Member

Choose a reason for hiding this comment

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

could we add this to a constant at the top of the file? and explain in code why this is needed?

Alternatively, where does innerTabContentHeight come from? Is there some alteration that needs to be made there instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some inveestigation about innerTabContentHeight, but no way to handle it dynamically.
I will add constant value at the top of the file. 👍

@etr2460
Copy link
Member

etr2460 commented Mar 16, 2022

Looks like CI is failing? If it's not related to your PR, maybe try rebasing?

@codemaster08240328
Copy link
Contributor Author

Looks like CI is failing? If it's not related to your PR, maybe try rebasing?

It is not related to my pr, also not related to rebasing stuff as well, I think rerunning may help?
Currently we have all PRs are failing in CI 🤔

@@ -152,7 +154,7 @@ export default function SouthPane({
query={latestQuery}
actions={actions}
user={user}
height={innerTabContentHeight}
height={innerTabContentHeight + EXTRA_HEIGHT_RESULTS}
Copy link
Contributor

@diegomedina248 diegomedina248 Mar 17, 2022

Choose a reason for hiding this comment

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

@rusackas we should add a story to rework these components, as they should use flexbox instead of manually calculating heights

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add one into the backlog. I seem to recall looking at this before, but there was some sort of wrinkle in implementing the library used to allow draggable resizing of the top/bottom panes.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, good call. We can take a look down the road

@rusackas
Copy link
Member

Actually this needs a rebase, and note that the conflict is due to some new styles... one of which has a height: 100%, so that miiiiiiight play a role here.

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #19138 (75af36f) into master (a08f83b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 75af36f differs from pull request most recent head 23fe92e. Consider uploading reports for the commit 23fe92e to get more accurate results

@@           Coverage Diff           @@
##           master   #19138   +/-   ##
=======================================
  Coverage   66.64%   66.64%           
=======================================
  Files        1674     1674           
  Lines       64614    64615    +1     
  Branches     6502     6502           
=======================================
+ Hits        43061    43062    +1     
  Misses      19868    19868           
  Partials     1685     1685           
Flag Coverage Δ
javascript 51.31% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...frontend/src/SqlLab/components/SouthPane/index.tsx 65.62% <100.00%> (+1.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a08f83b...23fe92e. Read the comment docs.

@rusackas rusackas merged commit 8947eb9 into apache:master Mar 24, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
* Remove gap from SQLLab results bottom

* resolve comment

(cherry picked from commit 8947eb9)
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* Remove gap from SQLLab results bottom

* resolve comment
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 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 lts-v1 Preset-Patch size/XS 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants