search

rogeriopvl / nodo

☑ Command line TODO app
http://rogeriopvl.github.io/nodo
MIT License
72 stars 4 forks source link

Fix&enhancement for issue #3 #4

Closed andreineculau closed 10 years ago

andreineculau commented 12 years ago

Misses the "default list" feature though. Much DRYer code but should still be improved. The original code was insane!

// Failed to get hub attach the pull-request to issue #3. Sorry

rogeriopvl commented 12 years ago

Hi! Thanks for the pull request. Nice improvements there.

I like most of the commits, but I think that new style of the output is not a good idea.

For instance, you used grey color, wich I'm unable to see in my terminal that has a grey-blueish background color (solarized theme). Also you indented lines, wich breaks lines if I have a small window terminal.

I try to assume very little about the colors and size of the user's terminal, because that's a tricky area. I used some strong colors that are unusual to be used as the background color, like blue, red and yellow, although blue is already too risky. Also I avoided some indentation to minimize the area of info display.

I'll get on this with more detail at the end of the day. Meanwhile it would be helpful if you placed the "comestic enhancements" in a separate commit. I like the templating part, so that code would be merged because its useful for faster display customisation (it would be interesting to have display colors and indentations configurable in nodorc, that would be a feature worth thinking about).

andreineculau commented 12 years ago
  1. colors -- good point, but i look around me, and I see the other devs being 50-50 on bright vs dark screens. You used yellow, and that does not work well at all on white backgrounds. If indeed it's an issue, just place the colors inside the nodorc as configs
  2. the indentation (I assume the padding) can be improved, for sure e.g. get a list of all lists, and pad as needed not up to some abstractly chosen number (like I did), but to the lengthiest list name. Or place these inside config as well. Otherwise, it's again subjective - you assume that the majority of people cannot afford more than 80 characters per line, and thus output very condensed information with no visual point of reference
  3. Sorry to say, but I won't touch nodo in the nearest future. Mostly because of lack of time. Feel free to use (&amend) or ignore this pull request. Thanks for understanding.
rogeriopvl commented 12 years ago
  1. Yes, indeed the yellow color is a problem. It's easy to overlook these small issues. I'll add the colors customisation in nodorc.
  2. I'll make this configurable in nodorc also.
  3. I understand, I'm also out of time and I'll try to merge your changes as soon as possible, probably will be doing some cherry-picking to enable nodorc configurable colors and padding. And again, thanks for your contribution :)
andreineculau commented 10 years ago

closing due to inactivity