Skip to content
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

feat(cmf): add two state static accessors #1371

Merged
merged 18 commits into from May 28, 2018

Conversation

jmfrancois
Copy link
Collaborator

@jmfrancois jmfrancois commented May 25, 2018

What is the problem this PR is trying to solve?

Every time we need to modify a state from the outside (saga, actionCreator) we need to deal with a low level api (actions.component.mergeState).
That means the dev need to know and hard code the name of the component and this is not easy, error prone and not futur proof (add HOC case for example which change the name).

What is the chosen solution to this problem?

Expose two static functions:

  • function getState(reduxState, id='default')
  • function setStateAction(componentState, id='default', type='ComponentName.setState')

Please check if the PR fulfills these requirements

  • The PR commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features) And non reg done before need review
  • Docs have been added / updated (for bug fixes / features)
  • Related design / discussions / pages (not in jira), if any, are all linked or available in the PR

[ ] This PR introduces a breaking change

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@acateland
Copy link
Contributor

@jmfrancois what happen if i have multiple instance of this component ? how to deal with such situation ?

…ub.com:Talend/ui into jmfrancois/feat/cmf/add-static-state-accessors
@jmfrancois
Copy link
Collaborator Author

I have updated the doc to show you how

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

1 similar comment
@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@@ -221,6 +221,32 @@ If you want to render some component conditionally, just pass "renderIf" prop (t
You can also use Expression for this and customize this prop like "renderIfExpression" in
CMF json configuration files

## How to access state and mute from the outside
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, by 'mute' do you mean 'mutate'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think that is what he mean, but it is also misleading since the state is not mutated but updated with a non mutable operation.
## Read and update component state from outside

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -26,5 +32,5 @@ The react/require-extension rule is deprecated. Please use the import/extensions
/home/travis/build/Talend/ui/packages/cmf/src/sagas/collection.js
10:1 error Prefer default export import/prefer-default-export

12 problems (9 errors, 3 warnings)
18 problems (15 errors, 3 warnings)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm some cleaning maybe ;)

static getState = function getState(state, id = 'default') {
return state.cmf.components.getIn([getComponentName(WrappedComponent), id], defaultState);
};
static setStateAction = function setStateAction(state, id = 'default', type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state parameter to be renamed newState i think
also shouldn't those state update operation also support synchronous state update function ? for safe state update operation when relying on previous state ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newState already exists in the upper declaration
I have added the asyn state update support

@@ -221,6 +221,32 @@ If you want to render some component conditionally, just pass "renderIf" prop (t
You can also use Expression for this and customize this prop like "renderIfExpression" in
CMF json configuration files

## How to access state and mutate from the outside
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

title may be misleading, the state is not mutated in sense of pointer value mutation, but it is updated
to avoid confusion i would rename this title to
## Read and update component state from outside

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@jmfrancois
Copy link
Collaborator Author

I need #1374 to let the tests pass

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

1 similar comment
@build-travis-ci
Copy link
Collaborator

:octocat: Demo is available here

@jmfrancois jmfrancois merged commit 75f4211 into master May 28, 2018
@jmfrancois jmfrancois deleted the jmfrancois/feat/cmf/add-static-state-accessors branch May 28, 2018 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants