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.

Sign up for GitHub

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

Jump to bottom

fix(explore): Icons width #14717

Merged
merged 3 commits into from
May 21, 2021
Merged

fix(explore): Icons width #14717

merged 3 commits into from
May 21, 2021

Conversation

geido
Copy link
Member

@geido geido commented May 19, 2021 •
edited
Loading

SUMMARY

It reverts a change of width and margin introduced in #14674 and adjusts the width of the icons in the options labels.

Fixes #14697

BEFORE

Screen Shot 2021-05-19 at 20 09 18

AFTER

Screen Shot 2021-05-19 at 20 04 37

TEST PLAN

  1. Open a chart in Explore
  2. Make sure the icons have proper paddings in the controls

ADDITIONAL INFORMATION

  • Has associated issue: [Explore] Explore column icon got lost in icon color change #14697
  • 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 19, 2021 •
edited
Loading

Codecov Report

Merging #14717 (c9ad13f) into master (be8c176) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #14717    +/-   ##
========================================
  Coverage   77.53%   77.53%            
========================================
  Files         959      959            
  Lines       48648    48701    +53     
  Branches     5734     6115   +381     
========================================
+ Hits        37718    37761    +43     
- Misses      10729    10734     +5     
- Partials      201      206     +5
Flag Coverage Δ
javascript 72.51% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...plore/components/controls/OptionControls/index.tsx 90.42% <ø> (ø)
...rontend/src/explore/components/optionRenderers.tsx 100.00% <100.00%> (ø)
...ontrols/AnnotationLayerControl/AnnotationLayer.jsx 83.40% <0.00%> (-0.88%) ⬇️
...mponents/controls/AnnotationLayerControl/index.jsx 7.79% <0.00%> (-0.55%) ⬇️
...l/AdhocFilterEditPopoverSimpleTabContent/index.jsx 78.91% <0.00%> (-0.48%) ⬇️
superset-frontend/src/components/Form/FormItem.tsx 100.00% <0.00%> (ø)
...erset-frontend/src/datasource/DatasourceEditor.jsx 66.38% <0.00%> (ø)
...end/src/SqlLab/components/RunQueryActionButton.tsx 64.28% <0.00%> (ø)
...d/src/filters/components/GroupBy/transformProps.ts 33.33% <0.00%> (ø)
...src/filters/components/TimeGrain/transformProps.ts 33.33% <0.00%> (ø)
... and 7 more

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 be8c176...c9ad13f. Read the comment docs.

@geido
Copy link
Member Author

geido commented May 19, 2021 •
edited
Loading

We had a conversation with @michael-s-molina to discuss the reason for the original change. By reverting the changes as I did in this PR, the related issue gets fixed but the padding of the icons in the GROUP BY control will be slightly affected. In order to avoid the back and forth, I'll resolve that problem as well in this PR and I'll update the status.

@junlincc junlincc added the bash! label May 19, 2021
@apache apache deleted a comment from github-actions bot May 20, 2021
@apache apache deleted a comment from github-actions bot May 20, 2021
@geido
Copy link
Member Author

geido commented May 20, 2021

This should be good to go now. CC @michael-s-molina @rusackas

@michael-s-molina
Copy link
Member

@geido Thanks for the fix. I have only a small suggestion to increase the margin-right of Metrics and Group by to gridUnit * 2 because the f icon is really small. This is how it will look like:

Before:
Screen Shot 2021-05-20 at 7 34 57 AM

After:
Screen Shot 2021-05-20 at 7 29 28 AM

@geido
Copy link
Member Author

geido commented May 20, 2021

@geido Thanks for the fix. I have only a small suggestion to increase the margin-right of Metrics and Group by to gridUnit * 2 because the f icon is really small.

Done!

@junlincc junlincc added the explore Namespace | Anything related to Explore label May 21, 2021
@junlincc
Copy link
Member

@michael-s-molina thanks for reviewing! can you review/test this PR locally with drag and drop enabled and on screen resize? Thanks! 🙏

@michael-s-molina
Copy link
Member

@michael-s-molina thanks for reviewing! can you review/test this PR locally with drag and drop enabled and on screen resize? Thanks! 🙏

screen-recording-2021-05-21-at-15810-pm_i4FrkZZi.mov

rusackas
rusackas approved these changes May 21, 2021
View reviewed changes
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! Thank you!

@rusackas rusackas merged commit fb6d21a into apache:master May 21, 2021
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request May 25, 2021
Merge branch 'master' into feat/fileter_set
ad54807 
* master: (163 commits)
  fix(native-filters): Manage default value of filters by superset (apache#14785)
  fix: Additional ResultSet tests (apache#14741)
  chore: added BasicParametersMixin to Redshift (apache#14752)
  fix: make dataset list sort case insensitive (apache#14528)
  fix: use encodeURIComponent when getting table metadata (apache#14790)
  fix: ensure engine is outside parameters (apache#14787)
  database modal should close on connect with tab layout (apache#14771)
  feat(native-filters): add search all filter options (apache#14710)
  fix: extra query in Dashboard when native filter enabled (apache#14770)
  chore: Improves the native filters UI/UX - iteration 2 (apache#14753)
  fix(native filters): Fix explore state (apache#14779)
  fix(explore): DndColumnSelect not handling controls with "multi: false" (apache#14737)
  feat: Create BigQuery Parameters for DatabaseModal (apache#14721)
  feat: enable user impersonation in GSheets (apache#14767)
  fix: add DB should not say it's Postgres (apache#14766)
  Revert "fix(dashboard): multiple query trigger when native filter enabled (apache#14734)" (apache#14762)
  feat: save database with new dynamic form (apache#14583)
  fix: save non-parameter DBs (apache#14759)
  chore: Removes ColorSchemeControl.less (apache#14199)
  fix(explore): Icons width (apache#14717)
  ...
@junlincc
Copy link
Member

Screen Shot 2021-05-25 at 3 39 55 PM

Im still seeing this issue on master. in table chart. @geido

@junlincc
Copy link
Member

junlincc commented May 25, 2021 •
edited
Loading

appears that there are 2 separate icon issues, one in Explore select, one in Table formatter(which I reported). the second one was not fixed. please push a quick fix ASAP @geido Thanks

To reproduce, go to any table charts, add columns, go to customize tab CUSTOMIZE COLUMN. see cosmetic

cc @rusackas @michael-s-molina

@rusackas rusackas mentioned this pull request May 26, 2021
Closed
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@geido
fix(explore): Icons width (apache#14717)
e1b40e9 
* Revert width and margin

* Adjust width

* Increase margin
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@geido
fix(explore): Icons width (apache#14717)
120fc3c 
* Revert width and margin

* Adjust width

* Increase margin
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@geido
fix(explore): Icons width (apache#14717)
c9a53e1 
* Revert width and margin

* Adjust width

* Increase margin
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 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
Reviewers

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@rusackas rusackas rusackas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore Namespace | Anything related to Explore size/XS 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Explore] Explore column icon got lost in icon color change
5 participants
@geido @michael-s-molina @junlincc @rusackas @mistercrunch