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: add action buttons to time series column popover #14525

Merged
merged 1 commit into from
May 13, 2021
Merged

fix: add action buttons to time series column popover #14525

merged 1 commit into from
May 13, 2021

Conversation

michael-s-molina
Copy link
Member

SUMMARY

Fixes #12672.
Improves the layout of the elements.

@rusackas will open another PR to globally fix the tooltip wrap. I increased the spacing to avoid wrapping.

@junlincc @zuzana-vej

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

105433267-27164300-5c0e-11eb-91e5-fb0bd1706089
Screen Shot 2021-05-07 at 10 34 18 AM

TEST PLAN

1 - Create a Time Series table
2 - Check Times Series Columns control

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@junlincc
Copy link
Member

junlincc commented May 8, 2021

/testenv up

@junlincc junlincc changed the title fix: Fixes #12672 fix: add action buttons to time series column popover May 8, 2021
@junlincc junlincc requested review from rusackas and pkdotson May 8, 2021 08:01
@junlincc junlincc added the bash! label May 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2021

@junlincc Ephemeral environment spinning up at http://54.186.165.68:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codecov
Copy link

codecov bot commented May 8, 2021 •
edited
Loading

Codecov Report

Merging #14525 (cb57ebd) into master (6d786d4) will increase coverage by 0.00%.
The diff coverage is 88.46%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14525   +/-   ##
=======================================
  Coverage   77.40%   77.40%           
=======================================
  Files         958      958           
  Lines       48329    48356   +27     
  Branches     5679     5682    +3     
=======================================
+ Hits        37410    37432   +22     
- Misses      10719    10724    +5     
  Partials      200      200
Flag Coverage Δ
javascript 72.48% <88.46%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...ponents/controls/TimeSeriesColumnControl/index.jsx 91.78% <77.77%> (-8.22%) ⬇️
superset-frontend/src/common/components/index.tsx 95.91% <100.00%> (+0.26%) ⬆️
.../src/explore/components/controls/BoundsControl.jsx 97.43% <100.00%> (+3.49%) ⬆️
superset-frontend/src/views/store.ts 100.00% <0.00%> (ø)
superset-frontend/src/dashboard/actions/hydrate.js 1.75% <0.00%> (ø)

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 6d786d4...cb57ebd. Read the comment docs.

@junlincc
Copy link
Member

/testenv up

@junlincc junlincc added the hold:testing! On hold for testing label May 10, 2021
@github-actions
Copy link
Contributor

@junlincc Ephemeral environment spinning up at http://52.33.59.89:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas
rusackas reviewed May 11, 2021
View reviewed changes
superset-frontend/src/explore/components/controls/BoundsControl.jsx
this.setState({
minMax: [
Number.isNaN(this.props.value[0]) ? '' : this.props.value[0],
Number.isNaN(this.props.value[1]) ? '' : this.props.value[1],
Copy link
Member

Choose a reason for hiding this comment

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

This behaves a little funny in practice, in that if you type a non-numeric value into one of these fields, it doesn't complain, and the value sticks. However, that non-numeric value IS removed if you type into the other field.

In other words, you can enter a good value in min and then a bad value in max - that bad value is only removed if you go back and change min

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced Input by InputNumber. Now it only allows numeric values and the user can click on the spinner.

@michael-s-molina michael-s-molina force-pushed the fix-12672 branch 2 times, most recently from a4b2d08 to bd1637b Compare May 12, 2021 13:26
@junlincc junlincc self-requested a review May 13, 2021 01:27
@junlincc
Copy link
Member

tested locally. LGTM. Thanks again!

@junlincc junlincc added need:merge The PR is ready to be merged and removed hold:testing! On hold for testing labels May 13, 2021
@junlincc junlincc added the need:review Requires a code review label May 13, 2021
rusackas
rusackas approved these changes May 13, 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, thanks!

@rusackas rusackas merged commit d31958c into apache:master May 13, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@junlincc junlincc removed need:review Requires a code review need:merge The PR is ready to be merged labels May 18, 2021
@junlincc junlincc added the rush! Requires immediate attention label May 18, 2021
henryyeh pushed a commit to preset-io/superset that referenced this pull request May 18, 2021
@michael-s-molina @henryyeh
fix: Fixes apache#12672 (apache#14525)
b6ef99a 
(cherry picked from commit d31958c)
@junlincc junlincc removed the rush! Requires immediate attention label May 27, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@michael-s-molina
fix: Fixes apache#12672 (apache#14525)
8aa2899
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@michael-s-molina
fix: Fixes apache#12672 (apache#14525)
748f943
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@michael-s-molina
fix: Fixes apache#12672 (apache#14525)
2be77bf
@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

@rusackas rusackas rusackas approved these changes

@junlin-qa junlin-qa junlin-qa approved these changes

@pkdotson pkdotson Awaiting requested review from pkdotson

@junlincc junlincc Awaiting requested review from junlincc

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

Successfully merging this pull request may close these issues.

[explore] Time Series Columns Configuration - no save button, (i) wrapped to new line
5 participants
@michael-s-molina @junlincc @rusackas @junlin-qa @mistercrunch