-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
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 :) |
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! |
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. |
278da6c
to
7d3059b
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
a6f91b7
to
bd615c7
Compare
/testenv up |
@geido Ephemeral environment spinning up at http://34.219.155.104:8080. Credentials are |
There was a problem hiding this 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
superset-frontend/src/explore/components/controls/BoundsControl.tsx
Outdated
Show resolved
Hide resolved
bd615c7
to
32a7872
Compare
Ephemeral environment shutdown and build artifacts deleted. |
* chore: upgrade BoundsControl to TS,FC, add storybook * chore: improve React import reference consistency
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)
ofInputNumber
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No visual changes.
Values might be provided in the appropriate input form.
ADDITIONAL INFORMATION
@kgabryje , @mik-laj You might be interested in this PR!