Fixes #30638 - replace react-numeric-input with rc-number-input #7896
Conversation
|
Issues: #30638 |
|
[test foreman] |
There was a problem hiding this comment.
Thanks @yifatmakias, works well 👍
Doesn't it going to break to following css files?
https://github.com/theforeman/foreman/blob/2cd4d88297a36d9a43a34a64badb3b7aa18e7d31/webpack/assets/javascripts/react_app/components/CPUCoresInput/cpuCoresInput.scss
https://github.com/theforeman/foreman/blob/ae96cb70130968bdcc8b7a9a9af7edc7c6e0d9a8/webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/disk/disk.scss
webpack/assets/javascripts/react_app/components/common/forms/NumericInput.scss
Outdated
Show resolved
Hide resolved
yifatmakias
force-pushed
the
30638
branch
4 times, most recently
from
8998ce8 to
eed049e
Compare
|
[test foreman] |
|
This PR is blocked by another PR: |
|
[test katello] |
| .foreman-numeric-input-input-wrap { | ||
| width: 100%; | ||
|
|
||
| input { |
There was a problem hiding this comment.
We could drop the input styles if we could set the input className to form-control.
Unfortunately, the rc-input-number doesn't support custom class names on the input field.
Sent a PR to rc-input-number so they can have this feature:
react-component/input-number#262
Can you please add a comment in the code with a reminder to drop the input styles once merged?
| } | ||
|
|
||
| &:hover { | ||
| border-color: $input-border-hover; |
There was a problem hiding this comment.
When I set the disabled prop to true, the hover effect still exists and it looks odd.
| line-height: normal; | ||
|
|
||
| &:focus { | ||
| outline: $input-border-focus auto 0.5px; |
There was a problem hiding this comment.
I don't see the glowing effect that we usually have when focused.
try:
@import "~@theforeman/vendor/scss/mixins";
&:focus {
@include form-control-outline;
}
| line-height: normal; | ||
|
|
||
| &:focus { | ||
| @include form-control-outline; |
There was a problem hiding this comment.
Unexpected unknown at-rule "@include" at-rule-no-unknown
sharvit
added
Waiting on Packaging
|
Can we get ack from packaging please? |
.stylelintrc
Outdated
| @@ -3,7 +3,8 @@ | |||
| "stylelint-config-standard", | |||
| ], | |||
| "rules": { | |||
| "selector-type-no-unknown": null | |||
| "selector-type-no-unknown": null, | |||
| "at-rule-no-unknown": null | |||
There was a problem hiding this comment.
I'd be consistent and also add it here. You may also want to consider sorting the list alphabetically. Those are just some practices I started to use because it's easier to deal with when things get longer and you're searching for something.
There was a problem hiding this comment.
@ekohl Added another comma and also sorted alphabetically. Thanks for the review! :)
|
Oh, they finished. @sharvit I'm not sure you gave an ACK or not so leaving the final honors of merging to you. |
There was a problem hiding this comment.
Thanks @yifatmakias, looks good and works as expected 👍
Thanks @ekohl, merging.
The rc-input-number is a much simple library and has fewer bugs therefore it should replace the react-numeric-input in the NumericInput react component.