Skip to content

Conversation

@zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Mar 1, 2021

SUMMARY

  • move utils.ts and constants.ts to DateFilterControl.utils
  • rename DateFilterControl.frame to DateFilterControl.components
  • remove original DateFilterControl
  • implement time picker on filter box

TEST PLAN

changed files/packages structure only.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie requested review from ktmud and villebro March 1, 2021 03:13
@junlincc junlincc added the explore:refactor Related to refactoring Explore label Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #13377 (bb56099) into master (3c62069) will decrease coverage by 4.19%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13377      +/-   ##
==========================================
- Coverage   77.09%   72.90%   -4.20%     
==========================================
  Files         898      598     -300     
  Lines       45711    21088   -24623     
  Branches     5495     5386     -109     
==========================================
- Hits        35241    15374   -19867     
+ Misses      10344     5588    -4756     
  Partials      126      126              
Flag Coverage Δ
cypress 57.90% <100.00%> (+0.28%) ⬆️
hive ?
javascript 62.63% <71.42%> (+0.05%) ⬆️
mysql ?
postgres ?
presto ?
python ?
sqlite ?

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

Impacted Files Coverage Δ
...ols/DateFilterControl/components/AdvancedFrame.tsx 53.84% <ø> (ø)
...trols/DateFilterControl/components/CustomFrame.tsx 42.00% <ø> (ø)
...teFilterControl/components/DateFunctionTooltip.tsx 100.00% <ø> (ø)
...nts/controls/DateFilterControl/components/index.ts 100.00% <ø> (ø)
...ents/controls/DateFilterControl/utils/constants.ts 100.00% <ø> (ø)
...nts/controls/DateFilterControl/utils/dateParser.ts 93.58% <ø> (ø)
...-frontend/src/explore/components/controls/index.js 100.00% <ø> (ø)
...d/src/filters/components/Time/TimeFilterPlugin.tsx 0.00% <0.00%> (ø)
...nts/controls/DateFilterControl/DateFilterLabel.tsx 85.84% <100.00%> (ø)
...ols/DateFilterControl/components/CalendarFrame.tsx 84.61% <100.00%> (ø)
... and 307 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 3c62069...b88326b. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Here's FilterBox with the old component:
image
Here with the new one (probably needs a slight CSS tweak):
image
Also, should we just remove the old LegacyDateFilterControl, as I don't think it's really needed anymore after this?

@zhaoyongjie
Copy link
Member Author

Here's FilterBox with the old component:
image
Here with the new one (probably needs a slight CSS tweak):
image
Also, should we just remove the old LegacyDateFilterControl, as I don't think it's really needed anymore after this?

OH, thanks Ville, good catch. I will check this issue.

I very much agree to remove the old components, but I am afraid whether there are other companies that still rely on the original components.

@villebro
Copy link
Member

villebro commented Mar 1, 2021

I very much agree to remove the old components, but I am afraid whether there are other companies that still rely on the original components.

I think it's ok to remove it, as I feel we've made a collective decision to replace the old time picker with the new one. Anyway, if someone really needs it they can go to a previous SHA and get the code from there 🙂

@zhaoyongjie
Copy link
Member Author

I very much agree to remove the old components, but I am afraid whether there are other companies that still rely on the original components.

I think it's ok to remove it, as I feel we've made a collective decision to replace the old time picker with the new one. Anyway, if someone really needs it they can go to a previous SHA and get the code from there 🙂

OK. I remove these legacy codes in this PR.

@zhaoyongjie
Copy link
Member Author

zhaoyongjie commented Mar 1, 2021

@villebro, I fixed the style issue.

image

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM! Btw, I'd change the title to feat(viz): implement time picker on filter box or something similar, as this is more a new feature for filter box than just a refactor.

@junlincc junlincc added the need:merge The PR is ready to be merged label Mar 1, 2021
@zhaoyongjie zhaoyongjie changed the title refactor(explore): restructure DateFilterControl feat(viz): implement time picker on filter box Mar 1, 2021
@zhaoyongjie
Copy link
Member Author

Thank you @villebro.
I totally agree with your suggestion

@zhaoyongjie zhaoyongjie merged commit 488da65 into apache:master Mar 1, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* refactor(explore): restructure DateFilterControl

* fix UT

* remove datasource props

* remove unused files

* fix DateFilterControl in filterbox over length
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 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:refactor Related to refactoring Explore need:merge The PR is ready to be merged 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants