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: upgrade BoundsControl to TS #18200

Merged
merged 2 commits into from
Feb 8, 2022
Merged

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Jan 27, 2022

SUMMARY

Upgraded BoundsControl to TypeScript to apply direction outlined in #18100 .

Added storybook for BoundsControl.

In long term I will consider change emitted by onChange value to (number | undefined) to keep consistent with (number | undefined) of InputNumber.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

No visual changes.

Values might be provided in the appropriate input form.

ADDITIONAL INFORMATION

  • Has associated issue: see Migrate components to TypeScript #18100
  • Required feature flags: no
  • Changes UI: no
  • Includes DB Migration (follow approval process in SIP-59): no
    • Migration is atomic, supports rollback & is backwards-compatible: N/A
    • Confirm DB migration upgrade and downgrade tested: N/A
    • Runtime estimates and downtime expectations provided: N/A
  • Introduces new feature or API: no
  • Removes existing feature or API: no

@kgabryje , @mik-laj You might be interested in this PR!

@geido
Copy link
Member

geido commented Jan 27, 2022

Hey @ad-m just a hint, you don't need to check all the boxes under "Additional information" in your PR description. Just check those that apply :)

@geido
Copy link
Member

geido commented Jan 27, 2022

One suggestion for you, I'd say that smaller contributions are good but in order to keep things together and avoid fragmentation, we could group more work under one PR. For instance, what do you think about converting this component to a functional one? This PR seems the perfect place to do that!

Also, thank you very much for adding stories to Storybook. That's gold!

@ad-m
Copy link
Contributor Author

ad-m commented Jan 27, 2022

One suggestion for you, I'd say that smaller contributions are good but in order to keep things together and avoid fragmentation, we could group more work under one PR. For instance, what do you think about converting this component to a functional one? This PR seems the perfect place to do that!

I will convert it to FC. I can do it in this PR, and I can do it separately - as you prefer. I am open to doing whatever it takes to ensure that the review works efficiently and the work of the commiters is minimized.

I can see that this is not a technically attractive issue (but long-term important for the condition of the project, so perfect for someone who is just getting to know the structure of the project, like me), so it's harder to get a voluntary review, so I'm afraid that PR's like that will not get stuck. For reasons external to the project (personal), such a state of the matter would be uncomfortable for me, and small steps are always something forward.

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #18200 (32a7872) into master (4e2bdd4) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18200      +/-   ##
==========================================
- Coverage   66.30%   66.30%   -0.01%     
==========================================
  Files        1595     1595              
  Lines       62632    62628       -4     
  Branches     6309     6308       -1     
==========================================
- Hits        41529    41526       -3     
- Misses      19453    19454       +1     
+ Partials     1650     1648       -2     
Flag Coverage Δ
javascript 51.37% <88.88%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../src/explore/components/controls/BoundsControl.tsx 88.88% <88.88%> (ø)

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 4e2bdd4...32a7872. Read the comment docs.

@ad-m ad-m closed this Jan 29, 2022
@ad-m ad-m reopened this Jan 29, 2022
@geido
Copy link
Member

geido commented Jan 31, 2022

/testenv up

@github-actions
Copy link
Contributor

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

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

LGTM with a nit

@geido geido added the need:merge The PR is ready to be merged label Feb 8, 2022
@geido geido merged commit fa8c81e into apache:master Feb 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

Ephemeral environment shutdown and build artifacts deleted.

ofekisr pushed a commit to nielsen-oss/superset that referenced this pull request Feb 8, 2022
* chore: upgrade BoundsControl to TS,FC, add storybook

* chore: improve React import reference consistency
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 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 need:merge The PR is ready to be merged size/L 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants