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.

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

Add ability to toggle the commits graph via env variable #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zdoc01
Copy link

@zdoc01 zdoc01 commented Jun 25, 2017

Hey @notwaldorf! Thanks for taking the time to build and open source this awesome tool! I love it.

I tend to keep my terminal split into multiple panes, and I noticed the commits graph seems to have rendering issues when in smaller viewports, so I thought it would be helpful if I could easily hide it via environment variables (sort of aligns with #109). I do still like having the total number of commits easily visible for a given day and week, so I added them to the Today and Week box labels when the commits graph is hidden. Here's a quick look:

Before
screen shot 2017-06-25 at 11 30 51 am

screen shot 2017-06-25 at 11 45 27 am

After
screen shot 2017-06-25 at 11 32 46 am

screen shot 2017-06-25 at 11 44 48 am

The PR itself mostly just consists of introducing the new config value and adjusting the size of the boxes accordingly, as well as adding the total number of commits to the box labels.

Let me know what you think!

@mojoaxel
Copy link
Collaborator

@zdoc01 Thanks for your contribution!

Copy link
Collaborator

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

I like the idea of making it possible to disable parts of the app. Also using environment-variables it probably the right way to do that.

What I did not like is the way the "buildBoxes" function works. If such a big rewrite is needed it also should generalize the handling of all the other boxes and not only one.

@@ -45,25 +45,25 @@ variables have been set correctly, you can print them in the terminal -- for exa

All the settings the dashboard looks at are in the sample file `sample.env`. This file isn't used by the dashboard, it just
lists the environment variables that you can copy in your `rc` files:
- `TTC_BOTS` are the 3 twitter bots to check, comma separated. The first entry
- `TTC_BOTS` -- the 3 twitter bots to check, comma separated. The first entry
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with this is that this changes makes it realy difficult to review your PR.
Pull request should ideally only taggle on feature or issue and should not make any changes to code that is not part of this change-set.

I like the new style. could you create a new PR that "fixes" the README please!?

@@ -102,6 +101,62 @@ function tick() {
doTheCodes();
}

function buildBoxes(showCommitsGraph) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

because the buildBoxes manages multibe boxes the funtion parameter should not be that specific.
I would recommend removing the parameter and just use the global config internally.

@mojoaxel mojoaxel added 🎉 feature request Baby steps! ⏳ pending-response What are you waiting for? Christmas? labels Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 feature request Baby steps! ⏳ pending-response What are you waiting for? Christmas?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants