Conversation
arminakvn
requested review from
george-yihao-xu,
gouldingken,
nickcaler,
tadiraman and
timothyegale
| } | ||
| } | ||
| }, [block, blocks]); | ||
| }, [block, blocks,timeline.blocks.selected]); |
There was a problem hiding this comment.
why is the dependency not just block.selected?
There was a problem hiding this comment.
nice finding this took me so much to figure out.... so, it turned out in timeline, we can't simply expose the timeline context with the useTimeline... this because since we are having two context, one for timeline and another for the block itself, what happens is if the client, say in dashi, the user closes the timeline layout and say switches the layout to another layout, then after returning to the timeline layout, the two context go out of sync, because technically the block context won't exit with the timeline layout closing.... so my solution was this....
| blck.setGroupName(blck[this.groupBy]) | ||
| reslt[blck[this.groupBy]] = [blck] | ||
| @computed | ||
| get blockYIndecies() { |
| return (a_first_block > b_first_block) ? 1 : -1 | ||
| }).sort() | ||
|
|
||
| const overGroups = overSortedGroups.reduce((res, groupKey, i, allGroups) => { |
There was a problem hiding this comment.
generally I sould suggest avoiding large monolithic functions like this and breaking it down. If you break this down so that each of these larger variables is a computed property on the store, it looks cleaner, is more readable and you get the advantage that mobx will memoize the structure so that any changes only trigger recalculations where needed
| if (this.customSpaces) this.customSpaces.forEach((cs)=>{ | ||
| Object.keys(cs).forEach((k,i)=>{ | ||
| const time_span = cs[k] | ||
| Object.keys(cs['spaces']).forEach((k,i)=>{ |
There was a problem hiding this comment.
probably need a check for undefined here
| context.blocks.setAsSorted(); | ||
| } | ||
| } | ||
| }, [context.blocks, groupBy, groupByFieldName]); |
There was a problem hiding this comment.
since groupByFieldName is a ref, can it trigger the useEffect here?
|
Another thought, I think it will be beneficial to have storybook here to make the testing easier. Agree? So u dont need to connect to dashi app, right? |
| let grppRes = 0; | ||
| const grpd = this.all.reduce((reslt, blck) => { | ||
| if (Object.keys(reslt).includes(blck.attrProps[this.groupBy])) { | ||
| reslt[blck.attrProps[this.groupBy]].push(blck) |
|
No need to change in this PR. One thing we need to discuss as a team is to use correctly spelled variables. Exceptions can be new names or terms specific to a client. |
6 participants
attrPropobject to look at the values of the field it will need to group the blocks by.