Skip to content

Conversation

@suddjian
Copy link
Member

SUMMARY

There wasn't any margin, so when the filter values took up >= 3 lines, the gap disappeared. Now it doesn't.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
Screen Shot 2021-05-17 at 2 35 31 PM

After:
Screen Shot 2021-05-17 at 2 51 03 PM

Screen Shot 2021-05-17 at 2 51 37 PM

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #14679 (96aa17f) into master (2320bd4) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14679      +/-   ##
==========================================
- Coverage   77.55%   77.55%   -0.01%     
==========================================
  Files         958      958              
  Lines       48550    48551       +1     
  Branches     5703     5703              
==========================================
- Hits        37654    37652       -2     
- Misses      10696    10699       +3     
  Partials      200      200              
Flag Coverage Δ
hive 81.11% <ø> (ø)
javascript 72.52% <0.00%> (-0.01%) ⬇️
mysql 81.38% <ø> (ø)
postgres 81.40% <ø> (ø)
presto 81.10% <ø> (-0.01%) ⬇️
python 81.93% <ø> (-0.01%) ⬇️
sqlite 81.02% <ø> (ø)

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

Impacted Files Coverage Δ
...l/AdhocFilterEditPopoverSimpleTabContent/index.jsx 77.12% <0.00%> (-0.51%) ⬇️
superset/db_engine_specs/presto.py 89.89% <0.00%> (-0.43%) ⬇️

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 2320bd4...96aa17f. Read the comment docs.

@suddjian
Copy link
Member Author

I was torn on whether to go with gridUnit * 3 or gridUnit * 4 🤷

@suddjian
Copy link
Member Author

suddjian commented May 17, 2021

Related issue: #14570

@junlincc junlincc added the bash! label May 17, 2021
@junlincc
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment spinning up at http://54.214.220.163:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

I was torn on whether to go with gridUnit * 3 or gridUnit * 4 🤷

You went with gridUnit in general, and that makes me happy in and of itself.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://35.166.224.14:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

image

The ABC icon lost some margin or padding - not sure if that came from this PR somehow, or some other commit that came along for the ride.

@suddjian
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@suddjian Ephemeral environment spinning up at http://54.245.202.124:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@junlincc
Copy link
Member

junlincc commented May 18, 2021

Screen Shot 2021-05-17 at 3 18 17 PM

i think that's due to the icon migration work. it got lost at least few days ago. notrelated to this pr

@suddjian
Copy link
Member Author

Yes, the issue appears to exist in master as well, so not a problem with this PR.

@junlincc
Copy link
Member

@geido Diego, do you mind taking a look at the icon issue i mentioned above? thanks!

@geido
Copy link
Member

geido commented May 18, 2021

@geido Diego, do you mind taking a look at the icon issue i mentioned above? thanks!

Sure. I'll have a look

@geido
Copy link
Member

geido commented May 19, 2021

@junlincc let's not jump to conclusions, please 😂 This is not related to the icon migration but to this PR #14674. I am already working on a solution. Thanks

@junlincc
Copy link
Member

@junlincc let's not jump to conclusions, please 😂 This is not related to the icon migration but to this PR #14674. I am already working on a solution. Thanks

@geido i should have phrased it differently :) just an assumption not conclusion. please don't take it personally. thanks for the fix.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 26c0b30 into master May 19, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@rusackas rusackas deleted the fix-adhoc-filter-margin branch January 30, 2023 19:21
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 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 preset-io size/XS 🚢 1.3.0 First shipped in 1.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants