-
Notifications
You must be signed in to change notification settings - Fork 38
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
ISSUE #4721 - Change sequences to use viewpoints #5014
base: staging
Are you sure you want to change the base?
Conversation
frontend/src/v5/ui/routes/viewer/toolbar/selectionToolbar/selectionToolbar.component.tsx
Outdated
Show resolved
Hide resolved
frontend/src/v4/routes/viewerGui/components/sequences/sequences.component.tsx
Show resolved
Hide resolved
…as to seq player
}; | ||
|
||
export const fetchActivitiesDefinitionsSuccess = (state = INITIAL_STATE, { sequenceId, activities }) => { | ||
return { ...state, activities: {...state.activities, [sequenceId]: activities } }; | ||
state.activities = {...state.activities, [sequenceId]: activities } |
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 think can be just state.activities[sequenceId] = activities;
...state, | ||
sequences: sequencesList | ||
}; | ||
state.sequences = sequencesList; | ||
}; | ||
|
||
export const fetchActivitiesDefinitionsSuccess = (state = INITIAL_STATE, { sequenceId, activities }) => { |
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 you've refactored the code to use Immer, you no longer need to the set the default value for state, and do just
export const fetchActivitiesDefinitionsSuccess = (state = INITIAL_STATE, { sequenceId, activities }) => { | |
export const fetchActivitiesDefinitionsSuccess = (state, { sequenceId, activities }) => { |
This will also hold for all the other redux methods
@@ -151,6 +152,8 @@ export class SequencePlayer extends PureComponent<IProps, IState> { | |||
public componentDidUpdate(prevProps: IProps) { | |||
if (prevProps.value !== this.props.value) { | |||
this.setState({value: this.props.value}); | |||
|
|||
SequencesActionsDispatchers.prefetchFrames(); |
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.
Why did you move this one here?
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 player logic for showing viewpoints and that sort of things was moved into the player. This produces a cleaner code in sagas and more explicit "non-magic" code in the component
export const reset = () => { | ||
return {...INITIAL_STATE}; | ||
export const reset = (state) => { | ||
Object.assign(state, INITIAL_STATE); |
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 think this was easier to read as it was before, returning a clone of the INITIAL_STATE
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.
remember that with immer you CAN'T return a state, you just have to modify it
This fixes #4721
As well as https://github.com/3drepo/3D-Repo-Product-Team/issues/469
Description
Test cases