-
Notifications
You must be signed in to change notification settings - Fork 35
Pick action doesn't properly open included view #1046 #1055
Conversation
@@ -186,7 +186,9 @@ class DocumentList extends Component { | |||
this.setState({ | |||
cachedSelection: null | |||
}, () => { | |||
dispatch(setListIncludedView()); | |||
if (includedView.windowType === 'pickingSlot') { |
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.
@wiadev hmmm... not sure this one because 'pickingSlot' it's just one of the many windowTypes.
IMHO it shall work no matter which is the windowType.
If the process/action returned that "an included view shall be opened" then it shall be opened no matter what.
Btw, might be that i am not understanding the underlying problem from here. Pls feel free to discuss about that on chat or even voice call (preferable for me because it's faster).
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.
@teosarca
Yes, this piece of code looks very weird.
Please let me explain.
- Before my changes this line was intended to reload includedView contents when user clicks on another product line (on left side).
- But when we added overriding includedView upon click on "Select HU" action we've get triggering of this method call when we actually just replacing the includedView with a new one.
- So your assumption - "an included view shall be opened" then it shall be opened no matter what" is valid and it will work in the same way. The only exception - pickingSlot window, which has a bit different data flow and needs to clear contents (see line 189 before my changes) and then load new list of slots.
In fact this is workaround to avoid more massive changes in DocumentList.
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.
about 3. I am a bit concerned about that. While the fix is solving our current problem (which is in Picking modal popup) it's not solving it generally. And that's also important for us because these features are basically our building blocks for more and more functionalities (like LEGO :) ).
I think we shall create a new task to refactor and do it right. On that task we shall also discuss about it, i can also give you more input. We don't have any pressure on that. The "high prio" of this task was exactly because we need to deliver the Picking window ASAP.
wdyt?
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.
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.
As I mentioned, this is workaround. But doing this right requires significant rework of DocumentList and potentially has wide impact, so we need to do this carefully.
Distinct task for that would be good.
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.
=> #1057
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.
for the records: this PR was closed because the fix was provided on #1069
#1046