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

chore(explore): added tooltips to timepicker #12580

Merged
merged 6 commits into from
Jan 25, 2021

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jan 18, 2021

SUMMARY

The new timepicker introduce date expression function, so need some online help.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

image

TEST PLAN

Tested on latest Firefox

ADDITIONAL INFORMATION

@zhaoyongjie zhaoyongjie requested review from junlincc, graceguo-supercat and hughhhh and removed request for hughhhh January 18, 2021 09:33
@zhaoyongjie zhaoyongjie added explore:time Related to the time filters in Explore hold! On hold need:design-review Requires input/approval from a Designer labels Jan 18, 2021
@junlincc
Copy link
Member

@mihir174 we will need a better UI for timepicker advanced tooltip. can you provide design support in 2 steps, 1. giving some lightweight suggestions to ship this asap 2. incorporate design system guidelines for long term
thanks! 🙏

@codecov-io
Copy link

codecov-io commented Jan 18, 2021

Codecov Report

Merging #12580 (822ea5c) into master (9e58eb8) will decrease coverage by 0.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12580      +/-   ##
==========================================
- Coverage   66.73%   66.36%   -0.38%     
==========================================
  Files        1021     1022       +1     
  Lines       49967    49978      +11     
  Branches     4890     4891       +1     
==========================================
- Hits        33347    33168     -179     
- Misses      16491    16685     +194     
+ Partials      129      125       -4     
Flag Coverage Δ
cypress 50.92% <100.00%> (+0.40%) ⬆️
javascript 60.91% <61.53%> (+<0.01%) ⬆️
python 63.21% <ø> (-0.79%) ⬇️

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

Impacted Files Coverage Δ
...s/controls/DateFilterControl/DateFilterControl.tsx 85.32% <ø> (ø)
...controls/DateFilterControl/frame/AdvancedFrame.tsx 52.00% <100.00%> (+2.00%) ⬆️
...ls/DateFilterControl/frame/DateFunctionTooltip.tsx 100.00% <100.00%> (ø)
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/db_engine_specs/hive.py 54.61% <0.00%> (-29.24%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/connectors/sqla/models.py 84.31% <0.00%> (-6.28%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
... and 22 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 9e58eb8...a10ec06. Read the comment docs.

Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

can we keep the tooltip open, click outside to close so that user can refer to the content. later when we have the documentation ready, we should add link to the icon. lastly, can we use the same info icon with grey background?
(that whole thing just looks so ugly i can't stand it. 🤦🏾‍♀️) @mihir174

@mistercrunch
Copy link
Member

i18n! Let's wrap everything we can in {t('i18n')}

@mistercrunch
Copy link
Member

That's a huge tooltip! I'm wondering if we should link out to a new documentation page. Tooltip could say something like Superset supports powerful date/time expressions. For example [show a few examples], click to see learn more in the Superset documentation.

@mihir174
Copy link
Contributor

mihir174 commented Jan 20, 2021

@zhaoyongjie
If there's a page with the syntax info in Superset docs, we should link to that. If that doesn't exist yet, I think staying in context and having the huge modal makes sense for now.

For the tooltip:

  1. i think it would be better if the tooltip didn't cover the inputs - then the user could type while keeping the mouse hovered on the tooltip icon to refer to the syntax
  2. @junlincc a persistent tooltip breaks a lot of UI patterns and will be inconsistent. i think if the tooltip doesn't obscure the inputs, users should be able to refer to the content easily
  3. here's the correct tooltip icon - https://www.figma.com/file/ToYikUoHLg7fhIBUPZtZb0/Superset-Design-System?node-id=968%3A125277
  4. tooltip icon color should be Grey-4 (B2B2B2)

Slice 1

@junlincc
Copy link
Member

@srinify Srini, can either Robert or Daniel expedite getting documentation ready for this new feature? all description is written in feat(explore): time picker enhancement 🙏

@junlincc junlincc removed the need:design-review Requires input/approval from a Designer label Jan 20, 2021
@zhaoyongjie
Copy link
Member Author

@mihir174
Thank you for pointing these out. I changed the tooltip style again. please check the tooltip screenshot.

cc @junlincc

image

@junlincc junlincc removed the hold! On hold label Jan 24, 2021
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

tested again, good enough for now. ✅
one suggestion: Reset scroll position on popover reopen.
This is a nice to have, probably not worth too much time doing at this moment.

@graceguo-supercat @villebro can you both take a look and approve this change? we would like to get it in ASAP. 🙏

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the extra work, to make a perfect time picker!

@zhaoyongjie zhaoyongjie merged commit 307e3a9 into apache:master Jan 25, 2021
@graceguo-supercat
Copy link

FYI, I saw Superset had many docs here:
https://superset.apache.org/docs/miscellaneous/issue-codes#issue-1001
we can also share time picker cheatsheet there.

@zhaoyongjie
Copy link
Member Author

make time picker syntax to the miscellaneous section

cc: @junlincc

villebro pushed a commit that referenced this pull request Jan 25, 2021
* wip

* wip

* fix lint

* fix: tooltip cosmetic

* wip

* add license
@mistercrunch mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 explore:time Related to the time filters in Explore size/L v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants