Closed dignite closed 5 years ago
Thanks for the work on this, @dignite . I agree that this is a great feature.
Turns out that PR #18 was only closed because of the changes to history due to the force-push situation we had. (Hopefully now we won't see that again because we're being more deliberate about smaller changes.) I believe that @ThomasAwesome was planning to re-implement this once some of the other changes calmed down, but that should have been communicated publicly.
Since you took the trouble to resurrect it (and even including Thomas' git attributions, nice touch!), I think we'll move forward with it.
I did a quick look over it now and it seems good to me. But it would be nice to add a test for the shuffling in the timer-state.specs.js
so we have that coverage. No need to test the randomization there directly: just that the method is called on the mobbers and that the event is published.
I'll review it more closely later today or tomorrow.
The placement of the button is kind of bugging me, and I'm trying to decide if it's worth changing.
In the current placement, the shuffle button looks like it's the first mobber on the list (especially with the gray striping). If you have mobbers already, then maybe that's where you want the button. But if there aren't any mobbers yet then does it distract from adding them?
Maybe the button should go at the bottom, under the "Add" form? If you have a bunch of mobbers it would push the button down so you have to scroll though. But overall, I'm thinking I like it under better than on top... thoughts?
I tried both and settled on above, but I am okay either way. One idea could be to keep it where it is but disable/hide the shuffle if there are no mobbers
After trying it a few ways, I feel like the top placement improves without the gray background. The striping feels like it's part of the list of mobbers. But when it's white, it blends back into the page background and the first mobber has the gray background.
Only showing it for 2+ mobbers is an interesting idea. It gets it out of the way when you can't really use it and stops it from distracting new users from adding their mobbers. But then it may not be obvious why it's appearing / disappearing. Disabling for < 2 mobbers is a nicer touch, but doesn't solve the placement problem. /shrug
I'll try to get some other people to voice their opinions on the placement.
I'm fine with making that stripe white. I made it gray because of aestethics but UX trumps looks
I definitely don't like the gray striping, but I'm not a fan of the while either as there's no separation from this button's row and the mobber list. Is there another color that fits the UX theme that we can use to help distinguish this as distinct from the mobber list but also not bland?
mobberActions
to mobberListActions
to signal actions on the list as a wholetimer-state.spec.js
, mobbers dependency is not easily stubable with the current code. Not writing a test at this time but I think testing with a mocked mobbers
is a good idea once that is possible.Fair enough re: the test. I'll take a stab at it post-merge.
This is an attempt to revive #18 which is a great feature.
As a user of the app In order to mix things up And not having the same person that I am taking over from And not having the same person that I am handing over to And not having to remove and re-add mobbers to achieve this I want to be able to shuffle the order of the mobbers