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

Use monospace font in the superset timers #9709

Merged
merged 1 commit into from
May 6, 2020

Conversation

bkyryliuk
Copy link
Member

@bkyryliuk bkyryliuk commented May 1, 2020

CATEGORY

Choose one

  • Bug Fix

SUMMARY

Truncates timer to seconds, ms make ui very jiggly
More info in #9708

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:
jumpy_timer_sqllab
after
timer_in_seconds

TEST PLAN

Locally

ADDITIONAL INFORMATION

REVIEWERS

@bkyryliuk bkyryliuk force-pushed the bogdan/truncate_timer branch 2 times, most recently from aea24a5 to aeefe20 Compare May 1, 2020 17:20
@mistercrunch
Copy link
Member

We should use a monospace font of some kind too, the div space changing is a tiny regression from the recent font change. @rusackas FYI

@mistercrunch
Copy link
Member

mistercrunch commented May 1, 2020

Actually I wonder if anyone would ever care about sub-second reporting, maybe when testing / tweaking indexes (which I've done before in an OLTP context, along with EXPLAIN statements).

We could show the millisecs in on an tooltip / hover if we want to be perfectionists

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.

Actually on your gif I still see some width movement, let's use our monospace font (I forgot what it's called)

@john-bodley
Copy link
Member

john-bodley commented May 1, 2020

I agree with @mistercrunch regarding the monospace font. This should also resolve the need to truncate the sub-seconds.

@bkyryliuk
Copy link
Member Author

+1 for the monospace, I am not well familiar with this part of the codebase - where can I change it ?

@kingo55
Copy link
Contributor

kingo55 commented May 1, 2020

Might this be the spot?:

https://github.com/apache/incubator-superset/blob/dcbffed9bb1a9826952b0bc28323f599cb475264/superset-frontend/src/SqlLab/main.less#L160

I added font-family: monospace; to this clause and it makes it much less jittery:

Screen-Recording-2020-05-02-at-8

We have some existing monospace font families here though:

https://github.com/apache/incubator-superset/search?q=monospace&unscoped_q=monospace

@bkyryliuk
Copy link
Member Author

great suggestions, made it monospace:
timer_mono
cc @mistercrunch @john-bodley @kingo55

@bkyryliuk
Copy link
Member Author

@rusackas sure thing, done. Thanks

@bkyryliuk bkyryliuk changed the title Truncate timer to seconds Use monospace font in the superset timers May 5, 2020
Update superset-frontend/src/SqlLab/main.less

Co-authored-by: Evan Rusackas <evan@preset.io>
@mistercrunch mistercrunch reopened this May 6, 2020
@mistercrunch
Copy link
Member

close/reopen to trigger CI

@codecov-io
Copy link

codecov-io commented May 6, 2020

Codecov Report

Merging #9709 into master will decrease coverage by 4.74%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9709      +/-   ##
==========================================
- Coverage   70.72%   65.97%   -4.75%     
==========================================
  Files         584      585       +1     
  Lines       30381    30429      +48     
  Branches     3112     3115       +3     
==========================================
- Hits        21486    20077    -1409     
- Misses       8781    10168    +1387     
- Partials      114      184      +70     
Flag Coverage Δ
#cypress ?
#javascript 58.95% <ø> (+0.05%) ⬆️
#python 70.92% <ø> (+0.04%) ⬆️
Impacted Files Coverage Δ
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/SqlLab/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 146 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 858082f...83cabf0. Read the comment docs.

@mistercrunch
Copy link
Member

tenor

@mistercrunch
Copy link
Member

Had to restart the "Cypress" job in CI twice and wanted to leave a trace somewhere that this is happening.

@mistercrunch mistercrunch merged commit 292704f into apache:master May 6, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Feb 28, 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 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants