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

New 'position' option. Aligns the Progress Bar left, right or center. #22

Merged
merged 4 commits into from
Aug 9, 2018

Conversation

sidneys
Copy link
Contributor

@sidneys sidneys commented Aug 7, 2018

⛔️ Issue

  • Currently, every progressbar is aligned to the left, that is, justified to the screens left-hand side.
  • Centering or otherwise aligning the bar is not possible without hacks.

✅ Solution

  • This PR adds the positionoption, which can takes the values 'left' (default), 'right' or 'center'.
  • If used, any available whitespace is distributed to align the progressbar to the desired direction.

🗂 Type

  • 🍾 Feature

🔥 Severity

  • 💎 Non-Breaking Changes

🛃 Documentation

  • The documentation was extended to accomadate the new feature.

@AndiDittrich
Copy link
Member

HI Sidney,

thanks for your contribution! i will check/review it soon :)

i've seen that you've added 2 new external dependencies - are they required to provide some complex functionality or could they be replaced by a few lines of code ? intentionally i want to keep the number of dependencies as small as possible.

best regards, Andi

@AndiDittrich AndiDittrich changed the title [FEAT] New 'position' option. Aligns the Progress Bar left, right or center. New 'position' option. Aligns the Progress Bar left, right or center. Aug 7, 2018
@sidneys
Copy link
Contributor Author

sidneys commented Aug 7, 2018

Dear @AndiDittrich,

thanks for your quick response. I strongly suggest including the dependencies, as they help maintaining a well-tested tested and stable backbone for rendering things in somewhat quirky (cross platform) terminal environments. especially screen-width went through numerous iterations to get one use-case right :-)

Kind regards and thanks for a great module,
viele Grüße aus Berlin
S

@AndiDittrich
Copy link
Member

Hi Sidney,

i've some problems with screen-width ...

  • cli-progress is only visible in TTY environments - otherwise the progressbar is not displayed - therefore we didn't have to take care of other use cases
  • the implementation of screen-width is a bit strange because it uses the deprecated (<= nodejs 0.6) function getWindowSize on TTY or process.stdout
  • the current cross-platform solution is process.stdout.columns - in case the application is in tty mode, process.stdout will be an instance of tty.WriteStream - see https://nodejs.org/api/tty.html

best regards, Andi

@AndiDittrich AndiDittrich merged commit 3016b45 into npkgz:master Aug 9, 2018
AndiDittrich added a commit that referenced this pull request Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants