Added ability to toggle/minimize lanes #267
Conversation
app/containers/SwimlaneContainer.js
Outdated
| onTaskUpdated: (task, updateAsana) => { | ||
| dispatchProps.onTaskUpdated(task, id, currentProjectId, updateAsana); | ||
| }, | ||
| onSwimlaneToggle: (id) => { |
There was a problem hiding this comment.
The mergeProps function is only required when you need to access the container's state props in themapDispatchToProps as to my knowledge it isn't possible in mapDispatchToProps. As you don't need access the state props due to the swimlane id being passed through from the component the duplication isn't necessary as the function you declared above will be used. If you look at the end of mergeProps you'll see them all being merged together.
|
Hey, Thanks for the commit good work, just looking through now. Regarding the UI I think maybe a chevron approach my work better. Sadly our designer (@tomstarley) is on holiday at the moment so won't be able to chip in for a while. Possible option: |
app/reducers/ui.js
Outdated
| return Object.assign({}, state, { showTaskDetailsSidebarLoading: false }); | ||
| } | ||
| case 'TOGGLE_SWIMLANE': { | ||
| return document.querySelector('section[data-reactid*="'+action.payload.id+'"]').classList.toggle('swimlane--collapsed'); |
There was a problem hiding this comment.
Reducers aren't intended to update the UI in this way, what would be better is if each swimlane had a visible property, then the TOGGLE_SWIMLANE case would update the value accordingly. Then in Swimlane/index.jsxyou'd check the visible value for the prop and add the class there.
Let me know if you want some help with that 😄 .
For future reference reducers should only be used to update the state tree. http://redux.js.org/docs/basics/Reducers.html
There was a problem hiding this comment.
I originally had a property called swimlaneCollapsed that did this. The trouble I had was targeting specific lanes - I could expand/collapse all lanes, but not just one. I'll look at the docs you linked and keep trying :-)
|
Added a new commit with the property-based toggle I tried originally and the line updates you mentioned. The problem now is that TOGGLE_SWIMLANE doesn't target a specific swimlane - it acts as a collapse all. Can you help with that? |
| onTaskUpdated: (task, updateAsana) => { | ||
| dispatchProps.onTaskUpdated(task, id, currentProjectId, updateAsana); | ||
| }, | ||
| onSwimlaneToggle: (id, collapsed) => { |
There was a problem hiding this comment.
Is this still unnecessary? I don't think I fully understand what mergeProps and mapDispatchToProps do.
|
Good job, this definitely is more react/reduxy. At the moment the What you need to do is change the state on that one swimlane (aka section) entity. To do this you'll want to go into Finally, as this is a toggle you shouldn't have to pass in a boolean to specify which way to toggle, the view should receive the event, which in turn will run an action with the swimlane's id and then the reducer should toggle the current state like so Let me know if you need any help 😄 As a side note the project structure may seem confusing which is mainly down to me as I was learning react/redux whilst creating this project. I intend to clean it up when I get a chance. |
|
This is the first time I've used React/Redux so I'm learning too. This seemed like a small enough project with a small enough team that I could try to help out (and in doing so work on building my web dev skills 😄) and at the same time, this product is something my team could really use. We're a newly agile team in a department that has been mandated to use Asana for task management and I've been tasked with making it work. Kasban seems like it might be a good solution once it gets a little more fleshed out. |
|
Hey both! I'm back. How best can i review what's been done? Thanks for the help Tatianna! :) |
3 participants
Adds a button to the header of each lane that allows users to “hide” lanes they don't need.
Everything should match surrounding code styles, with the exception of line 39 of apps/reducers/ui.js. I couldn’t figure out how to target specific lanes using existing code, so there might be a better way to do it.