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.

Sign up for GitHub

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

extend event bus capabilities #417

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+24 −2
Open

extend event bus capabilities #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a119e2
feat: extend event bus
LucasBadico Nov 26, 2020
1a2287f
doc: update the readme with lane actions
LucasBadico Nov 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,19 @@ eventBus.publish({type: 'MOVE_CARD', fromLaneId: 'PLANNED', toLaneId: 'WIP', car
//To update the lanes
eventBus.publish({type: 'UPDATE_LANES', lanes: newLaneData})

//To update one lane
eventBus.publish({type: 'UPDATE_LANE', lane: { id: 'TODO', title: 'teste' }})
Copy link
Author

Choose a reason for hiding this comment

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

this one already was in code, but was not documented.


//To add a new lane
eventBus.publish({type: 'ADD_LANE', lane: { id: 'TODO', title: 'teste' }})

//To move one lane
eventBus.publish({type: 'MOVE_LANE', fromIndex: 0, toIndex: 1 })
Copy link
Author

@LucasBadico LucasBadico Nov 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm asking myself if it was not better to create a new action based on id, not on index. What do you think, @dapi?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @LucasBadico !

I agree with you. If we pass an id we can fetch lane's index when need.

What is you use case of MOVE_LANE event? Do you save lanes order to backend?

One thought came. When we drug one lane, from example move it from index 0 to index 5. Actually all lanes from 1 to 5 index are moved ) Shall we emit events for all these lanes? Probably better way to emit event like "LANES_MOVED" and pass new ordered lanes set?

Copy link
Author

Choose a reason for hiding this comment

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

You have some points.
but my use case here:

  • move lane was needed because the WebSocket and the feature of move lane and the socket keep the order of all clients synched.
  • would be a good thing to have that Lanes Moved, but now I'm doing this on my backend, with a head and tail logic.


//To remove a lane
eventBus.publish({type: 'REMOVE_LANE', laneId: 'TODO' })


<Board data={data} eventBusHandle={setEventBus}/>
```

Expand Down
13 changes: 11 additions & 2 deletions src/controllers/BoardContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,15 @@ class BoardContainer extends Component {
let eventBus = {
publish: event => {
switch (event.type) {
case 'REFRESH_BOARD':
return actions.loadBoard(event.data)

case 'ADD_CARD':
return actions.addCard({laneId: event.laneId, card: event.card})
case 'UPDATE_CARD':
return actions.updateCard({laneId: event.laneId, card: event.card})
case 'REMOVE_CARD':
return actions.removeCard({laneId: event.laneId, cardId: event.cardId})
case 'REFRESH_BOARD':
return actions.loadBoard(event.data)
case 'MOVE_CARD':
return actions.moveCardAcrossLanes({
fromLaneId: event.fromLaneId,
Expand All @@ -78,10 +79,18 @@ class BoardContainer extends Component {
})
case 'UPDATE_CARDS':
return actions.updateCards({laneId: event.laneId, cards: event.cards})

case 'UPDATE_LANES':
return actions.updateLanes(event.lanes)
case 'UPDATE_LANE':
return actions.updateLane(event.lane)
case 'MOVE_LANE':
return actions.moveLane({ oldIndex: event.fromIndex, newIndex: event.toIndex })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know fromIndex and toIndexnames are better then oldIndex and newIndex. But it is preferable not to create new entities. We need either rename current attributes either use their. As we can't simply rename their because of using them by users we just have to use current names.

@LucasBadico What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

make sense. Principle of least surprise.

case 'REMOVE_LANE':
return actions.moveLane({ laneId: event.laneId })
case 'ADD_LANE':
return actions.addLane(event.lane)

}
}
}
Expand Down