feat(containers/editabletext): create container #1601
Conversation
1 similar comment
| ...rest | ||
| }) { | ||
| if (loading) { | ||
| return <Skeleton type={Skeleton.TYPES.text} size={Skeleton.SIZES.large} />; | ||
| } | ||
|
|
||
| const Renderer = Inject.getAll(getComponent, { EditableText }); |
There was a problem hiding this comment.
Use const InjectedEditableText = Inject.get(getComponent, 'EditableText', EditableText)
| </small> | ||
| )} | ||
| {subTitle && ( | ||
| <small |
There was a problem hiding this comment.
Now this subTitle is rendered even if we are in edit mode. Is this wanted ?
There was a problem hiding this comment.
Now the SubHeader component has no way to know if its EditableText is in editMode or not. I don't see other solution, but if you have suggestions I'll be glad to ear it
| } | ||
| } | ||
|
|
||
| onChange(event) { |
There was a problem hiding this comment.
You removed all those methods but in the render you still refer to them instead of the props.
There was a problem hiding this comment.
ow, thank you, fixed
| * @param {object} state | ||
| * @param {string} idComponent | ||
| */ | ||
| export function getEditMode(state, idComponent) { |
There was a problem hiding this comment.
Be careful this is a breaking change. This can impact the apps. Please communicate on this after the merge and fill properly https://github.com/Talend/ui/wiki/BREAKING-CHANGE
| actionCreatorEdit: PropTypes.string, | ||
| actionCreatorSubmit: PropTypes.string, | ||
| actionCreatorChange: PropTypes.string, | ||
| actionCreatorGoBack: PropTypes.string, |
There was a problem hiding this comment.
This is not used and is proper to SubHeader not Editable Text
| } | ||
|
|
||
| onSubmit(event, data) { | ||
| this.props.setState(() => ({ |
There was a problem hiding this comment.
You don't need the function version, just call setState({ editMode: false })
| } | ||
|
|
||
| onCancel(event) { | ||
| this.props.setState(() => ({ |
|
|
||
| onEdit(event) { | ||
| this.props.setState(() => ({ | ||
| editMode: !this.props.state.get('editMode', false), |
There was a problem hiding this comment.
Same here, or take the state from the arguments instead of props.state
There was a problem hiding this comment.
BTW this seems weird in fact, don't you want to set editMode to true instead of toggle it ?
There was a problem hiding this comment.
oh you're probably right. I moved this code from the SubHeader container, but I agree, I don't see any reason to toggle it here. I corrected it, thanks.
| setState: jest.fn(), | ||
| }; | ||
| shallow(<Container {...props} />).simulate('submit', event, data); | ||
| expect(props.setState).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is not necessary, you already tested it in the previous test
| expect(props.onEdit).toHaveBeenCalledWith(event); | ||
| }); | ||
| it('should call onEdit when edit event trigger', () => { | ||
| // Given |
There was a problem hiding this comment.
Could you add the // given/when/then separators everywhere or remove them everywhere within the file ?
| import Container, { DEFAULT_STATE } from './EditableText.container'; | ||
|
|
||
| export default cmfConnect({ | ||
| componentId: ownProps => ownProps.id, |
There was a problem hiding this comment.
please remove this, here you mix component id and id.
They have two different purpose:
id is for HTML
componentId is for CMF
| props: this.props, | ||
| data, | ||
| }); | ||
| } |
There was a problem hiding this comment.
you need this ? onSubmitActionCreator should work (become a onSubmit)
There was a problem hiding this comment.
As discussed, this will cause a lot of breaking changes. Maybe we could do that in a second time...
| */ | ||
| export function getComponentState(state, idComponent) { | ||
| return state.cmf.components.getIn([DISPLAY_NAME, idComponent], DEFAULT_STATE); | ||
| } |
1 similar comment
| import Connect from './EditableText.connect'; | ||
| import { getEditMode } from './EditableText.selectors'; | ||
|
|
||
| describe('Connect', () => { |
There was a problem hiding this comment.
I might be wrong, but I don't feel that this has any testing purpose around the current container
2 similar comments
jsomsanith-tlnd
deleted the
ncomont/feat/containers/editabletext/create_container
branch
What is the problem this PR is trying to solve?
EditableText has no container.
What is the chosen solution to this problem?
Create it.
Please check if the PR fulfills these requirements
[ ] This PR introduces a breaking change