OBPIH-6024 Create a reusable TextInput component for Product Sources create/edit form#4496
Conversation
kchelstowski
left a comment
kchelstowski
left a comment
There was a problem hiding this comment.
Could you also please include some screenshots of your beautiful work? 😄
src/js/wrappers/InputWrapper.jsx
Outdated
| </div> | ||
| {redirect && ( | ||
| <div | ||
| onClick={() => history.push(redirect.redirectTo)} |
There was a problem hiding this comment.
We don't know on 100% if this is eventually always gonna be a React redirect, hence I think we should distinguish (maybe add an additional flag whether it's a "react link" or a gsp one to know, if we should use history.push or window.location).
There was a problem hiding this comment.
Good point, maybe a better approach (instead of passing an additional flag) will be to pass the whole "redirect" function, it will increase the usability of this button (in the context of the fact that we can pass different functions than just redirecting one)
There was a problem hiding this comment.
will be to pass the whole "redirect" function, it will increase the usability of this button
I was also considering such an approach, so it'd be fine to me.
src/js/wrappers/InputWrapper.jsx
Outdated
| </div> | ||
| {button && ( | ||
| <div | ||
| onClick={button.function} |
There was a problem hiding this comment.
| onClick={button.function} | |
| onClick={button.onClick} |
let's follow the react naming pattern for callback props that handle onClick functionality
src/js/wrappers/InputWrapper.jsx
Outdated
| id: PropTypes.string.isRequired, | ||
| defaultMessage: PropTypes.string.isRequired, | ||
| }), | ||
| // Hyperlink on the right side above the input |
There was a problem hiding this comment.
after your last change this comment doesn't correspond to the usecase of this prop.
| tooltip: null, | ||
| required: false, | ||
| title: null, | ||
| button: null, | ||
| errorMessage: null, |
There was a problem hiding this comment.
is there a reason we need to specify default prop values as null?
by default props that are not passed are undefined
There was a problem hiding this comment.
If a prop is not required, the linter shouts that we have to specify the default prop.
ESLint: propType "..." is not required, but has no corresponding defaultProps declaration.(react/require-default-props)
src/js/wrappers/InputWrapper.jsx
Outdated
| <div | ||
| onClick={button.function} | ||
| role="presentation" | ||
| className="input-wrapper-redirect" |
There was a problem hiding this comment.
after your last change input-wrapper-redirect is probably not an accurate description of this div
alannadolny
force-pushed
the
OBPIH-6024
branch
from
be7f7ca to
0f7e347
Compare
…create/edit form (#4496) * OBPIH-6024 Add text field and wrapper for input fields * OBPIH-6024 Add styling for text input field * OBPIH-6024 Change redirect argument to passed function * OBPIH-6024 Changes after review
4 participants
No description provided.