Closed yashvyas29 closed 5 years ago
Thanks for the details clearance @allan-stewart. We recently adopted mob programming for our company and I found your application is very useful for it. As you liked the application menu. Maybe I will generate a PR for that only in the future. I am trying the upgrade electron and other dependency versions also to the latest one but I am facing some issues. Let's see till when I can give some more time to it and fix it.
After taking some time to review this pull request, I've decided that it needs to be closed because the changes are too broad. I really like a few of them, but some are counter to the philosophy of this mob-timer while some other changes just feel random. Plus the tests and linter both fail.
I'll outline some of my reasoning below; hopefully it will help everyone understand my thinking. I'd be open to considering new PRs that include some of these new features, but only if they are smaller, more focused changes where each PR is just adding one thing (with passing tests!).
I hate to discourage people from contributing, but there are just too many problems with this PR in my opinion. Maybe we need to create a contribution guide?
Don't Show Exact Time Remaining The mob-timer used to show the amount of time remaining, similar to what was added in this PR. But we intentionally removed it because the circle that fills around the current mobber picture was a better indicator of time remaining than the digits themselves (e.g.
2:30
).When you are in a good mobbing flow, thinking about exactly how many seconds you have left can distract from getting actual work done. If you are the person at the keyboard, you might stop working prematurely. If you are the person next in the list, you don't need to know if you have exactly 5, 10, or even 30 seconds before your turn. You just need to know whether your turn is close or not.
Adding the exact time remaining would be acceptable to me if if was an optional feature (default off).
Windows Tray Icon I'm not sure what value this adds. As far as I can tell, it just gives you another way to shut down the program. Since it still lives in the task bar, I don't see the need for it. There is some code that calls
tray.setTitle(data.timeRemaining)
but I can't see where that actually displays on the tray. And clicking the tray icon doesn't move the timer to the foreground (when the timer isn't set to float above other windows).Close Button Is Counterproductive This PR adds a "Close" button to the fullscreen view. This feels unnecessary at best, since the window already closes when you press either "Start" or "Configure". Also, with this PR, the "Skip" button also makes the window disappear, which is undesirable since you might have to skip more than one person before you want to start the next turn.
But the biggest issue I have with the close button is that it is counterproductive to the concept of the fullscreen window. The whole point is that it fills the screen, forcing the team to switch places and start the next mobbing round. Allowing the user to press "Close" removes the indicators that they should get off the keyboard.
If you are done mobbing and the timer is in your way, just exit the program.
Window Frame This PR changes the window so there is a frame around it. This makes the timer unnecessarily larger, and doesn't look good to me. Unfortunately it is necessary on Windows if you want to show the menu, but shouldn't be shown on Mac OS, IMO.
Menu Items On Mac OS, the addition of the menu items is really great. Mostly this is because of how Mac OS handles menus -- the menubar is always at the top of the screen for every program. Having the standard "Preferences" item that opens the configuration screen is a very nice touch. And the help menu could have been really great -- except that it opens a webpage that has nothing to do with the mob-timer, mob-programming, or anything of real use. :(
On Windows, the menubar is not nearly as elegant. It only shows when you hit
Alt
and then the timer no longer fits inside its window. And to make it work you have to show the window frame, which is unfortunate. The value here really doesn't seem to outweigh what we'd be giving up.Misc
There are some good ideas in this PR, but they need to be submitted at separate PRs with passing tests before I think they could be accepted.