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

style: fix labels vertical align #11415

Merged
merged 4 commits into from
Oct 26, 2020
Merged

style: fix labels vertical align #11415

merged 4 commits into from
Oct 26, 2020

Conversation

mistercrunch
Copy link
Member

SUMMARY

recently merged PR #11400 messed up the vertical alignment of <Label /> in some contexts.

BEFORE

Screen Shot 2020-10-23 at 9 49 50 PM

Screen Shot 2020-10-23 at 9 49 45 PM

TEST PLAN

visual

@codecov-io
Copy link

codecov-io commented Oct 24, 2020

Codecov Report

Merging #11415 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11415      +/-   ##
==========================================
+ Coverage   66.49%   66.55%   +0.05%     
==========================================
  Files         860      860              
  Lines       40869    40871       +2     
  Branches     3686     3688       +2     
==========================================
+ Hits        27176    27201      +25     
+ Misses      13592    13571      -21     
+ Partials      101       99       -2     
Flag Coverage Δ
#cypress 56.56% <ø> (+0.20%) ⬆️
#javascript 62.92% <ø> (-0.01%) ⬇️
#python 61.94% <ø> (ø)

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

Impacted Files Coverage Δ
...erset-frontend/src/components/DatabaseSelector.tsx 91.30% <ø> (ø)
superset-frontend/src/components/Label/index.tsx 100.00% <ø> (ø)
superset-frontend/src/components/Timer.tsx 95.83% <ø> (ø)
...et-frontend/src/dashboard/actions/sliceEntities.js 78.78% <0.00%> (-1.86%) ⬇️
.../src/dashboard/components/gridComponents/Chart.jsx 83.67% <0.00%> (-1.03%) ⬇️
...ntend/src/dashboard/components/PropertiesModal.jsx 90.00% <0.00%> (-0.91%) ⬇️
...-frontend/src/dashboard/components/SliceHeader.jsx 58.33% <0.00%> (ø)
...src/dashboard/components/HeaderActionsDropdown.jsx 68.88% <0.00%> (ø)
...t-frontend/src/dashboard/components/SliceAdder.jsx 98.50% <0.00%> (+1.49%) ⬆️
...set-frontend/src/dashboard/util/getDropPosition.js 93.65% <0.00%> (+1.58%) ⬆️
... and 4 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 efdda8b...d06dad3. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Oct 24, 2020

Just for the record, this PR also fixes the alignment of the datasource menu trigger on Explore page.

Before

(misaligned by #11400 )

After

BTW...

Can we also add some space between the pill and the database name?

This label on the Edit Datasource modal also seem to need some adjustment

@mistercrunch
Copy link
Member Author

How about this?
Screen Shot 2020-10-24 at 11 16 23 AM
Screen Shot 2020-10-24 at 11 16 12 AM
Screen Shot 2020-10-24 at 11 16 08 AM
Screen Shot 2020-10-24 at 11 16 03 AM

@ktmud
Copy link
Member

ktmud commented Oct 24, 2020

I think text in brackets looks too hacky as it reminds me of text-based UI instead of GUI. Maybe we can create a special label with less padding for this?

@mistercrunch
Copy link
Member Author

mistercrunch commented Oct 24, 2020

Ok, I'm throwing in the space for now but will save the Label work for another PR.

My take on Label is that it probably needs a border in all cases (not just the clickable labels), and that we can use box-shadow to differentiate the clickable ones.

Also, part of the reason that Labels were uppercase in the past is that it helps with the vertical alignment perception. Without uppercase, text looks pushed down - not vertically aligned in many cases.

@@ -30,8 +30,8 @@ interface TimerProps {
}

const TimerLabel = styled(Label)`
width: 80px;
text-align: right;
width: 96px;
Copy link
Member

Choose a reason for hiding this comment

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

If this used the fixed-width / tabular-numerals variant if Inter, we could probably just as easily specify the width in 'em' units, but this seems like it'd work just fine for now, and I'll try to remember to open a PR to address these tabular numerals in the Theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right, em seems like the way to go here!

@rusackas rusackas merged commit 0ab1537 into apache:master Oct 26, 2020
@rusackas rusackas deleted the fix_labels branch October 26, 2020 00:55
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* style: fix labels vertical align

* addressed comments

* just adding the space for now

* fix timer
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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 size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants