-
Notifications
You must be signed in to change notification settings - Fork 35
Implement selections per windowType and viewId #1264
Conversation
@pablosichert hi Pablo, can u pls review this one? |
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 changes are a good move towards reducing complexity of the codebase. 👍
(There is still a lot to be done, especially restructuring the store so components can subscribe to changes properly.)
modalSaveStatus={ | ||
modal.saveStatus && | ||
modal.saveStatus.saved !== undefined ? | ||
modal.saveStatus.saved : true |
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 would reverse this condition for readability:
modal.saveStatus &&
(modal.saveStatus.saved === undefined ?
true : modal.saveStatus.saved)
However, I can't imagine why it should return true
when saved
is undefined
.
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.
not sure if is related, but the "saveStatus" (same as validStatus) are provided only when it's changed.
So it might be that saveStatus is missing from API response, just because nothing changed.
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.
That's most likely the correct explanation, thanks
}; | ||
|
||
export const NO_SELECTION = []; | ||
export const getSelection = ({ state, windowType, viewId }) => { |
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.
Ideally, windowType
and viewId
should be managed and taken from the store directly. Currently viewId
is managed in DocumentList
's internal state which is hard to share between all components reliably. When it's only passed down as a prop, we can't share it with sibling or parent components and lose the overview quick.
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 problem is if we keep windowType
and viewId
in store, there still are different windowType
s and viewId
s per DocumentList
. And there can be 4 different DocumentList
s active at the same time: modal and non-modal, and additionally as includedView
each.
But you are right, viewId
can not be shared with components other than its children. Actually this line (48) https://github.com/metasfresh/metasfresh-webui-frontend/blob/a49cbf8d11878b885d74a1e833c5e4865b51d6b8/src/components/app/DocumentList.js#L44-L50 uses defaultViewId
from props
instead of state
, because there is no way to access state
or context
in connect()
...
This is confusing, but not because of this connect()
but because of the existence of defaultViewId
. The viewId
from state
is only changed here:
- In
constructor()
: equals todefaultViewId
:
https://github.com/metasfresh/metasfresh-webui-frontend/blob/a49cbf8d11878b885d74a1e833c5e4865b51d6b8/src/components/app/DocumentList.js#L65-L69 - In
componentWillReceiveProps()
:viewId
was retrieved fromstate
https://github.com/metasfresh/metasfresh-webui-frontend/blob/a49cbf8d11878b885d74a1e833c5e4865b51d6b8/src/components/app/DocumentList.js#L123-L138 - In
componentWillReceiveProps()
as well:defaultViewId
fromnextProps
, soconnect()
will update its selection
https://github.com/metasfresh/metasfresh-webui-frontend/blob/a49cbf8d11878b885d74a1e833c5e4865b51d6b8/src/components/app/DocumentList.js#L153-L157 - In
createView()
: This function is called, if noviewId
is given, in which caseconnect()
can not retrieve any selections anyway. I think this point might be an issue, as the selection will be invalid in this case!
https://github.com/metasfresh/metasfresh-webui-frontend/blob/a49cbf8d11878b885d74a1e833c5e4865b51d6b8/src/components/app/DocumentList.js#L314-L324
(See #1248)
There was a lot of refactoring needed to reduce the complexity of the gist of this PR: a49cbf8
Now there is one global
state.windowHandler.selected
andselectedWindowType
attribute shared across all windows.DocumentList
caches the selection to avoid losing selection:https://github.com/metasfresh/metasfresh-webui-frontend/blob/25ecf1f3c0716c7312c47e63ee6c16d51157c021/src/components/app/DocumentList.js#L158-L185
With this PR, this part will be removed. Instead
selections
for eachwindowType
andviewId
are stored, meaning selections can be re-used after changing view (and not being re-created in future).Also the components
DocumentList
,Modal
andSubHeader
will retrieveselected
items based on theirviewId
andwindowType
props.