-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor: convert FilePicker into functional component #1235
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1235 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 93 93
Lines 1377 1361 -16
Branches 344 339 -5
=========================================
- Hits 1377 1361 -16
Continue to review full report at Codecov.
|
7857247
to
ff8f585
Compare
src/components/FilePicker/index.jsx
Outdated
const FilePicker = ({ filter, dts, placeholder, onSelect, onRemove, isHighlighted, disabled, label }) => { | ||
const fileInputRef = React.useRef(); | ||
const [fileName, setFileName] = React.useState(''); | ||
const [isFileSelected, setIsFileSelected] = React.useState(false); |
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.
i think this state is redundant. maybe you can have a look see if you can remove it
www/containers/props.json
Outdated
@@ -957,13 +957,13 @@ | |||
"methods": [ | |||
{ | |||
"name": "usePreventSwipeClicks", | |||
"docblock": "A hook to prevent child click handlers from firing immediately after swiping the Carousel\n\n@returns {object} - handlers to be spread onto any carousel items with `onClick` handlers", |
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.
why is this change here?
@@ -11,7 +11,6 @@ | |||
background-color: $color-disabled; | |||
border: 0; | |||
box-shadow: none; | |||
height: 28px; |
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.
context for the css change?
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.
the two buttons (X & select) have the different height, so I removed here and added the height into .aui--button
selector which can be applied for two buttons for alignment. Should I just leave the style as the same as before?
ff8f585
to
f7107de
Compare
f7107de
to
9fef8ea
Compare
Description
Does this PR introduce a breaking change?
Manual testing step?
Screenshots (if appropriate):