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

progress bar (WIP) #3

Closed
wants to merge 12 commits into from
Closed

Conversation

brigand
Copy link
Contributor

@brigand brigand commented Jul 8, 2017

Hey, cool project!

I'm working on adding a progress bar component, but getting a weird error:

$ ./examples/run progress
/Users/fb/github/brigand/ink/lib/instance.js:10
const isClassComponent = component => component.prototype instanceof Component;
                                               ^

TypeError: Cannot read property 'prototype' of undefined
    at isClassComponent (/Users/fb/github/brigand/ink/lib/instance.js:10:39)
    at new Instance (/Users/fb/github/brigand/ink/lib/instance.js:40:27)
    at getOrCreateInstance (/Users/fb/github/brigand/ink/lib/instance.js:20:14)
    at children.forEach.child (/Users/fb/github/brigand/ink/lib/instance.js:84:27)
    at Array.forEach (native)
    at Instance.mount (/Users/fb/github/brigand/ink/lib/instance.js:82:17)
    at children.forEach.child (/Users/fb/github/brigand/ink/lib/instance.js:87:19)
    at Array.forEach (native)
    at Instance.mount (/Users/fb/github/brigand/ink/lib/instance.js:82:17)
    at diff (/Users/fb/github/brigand/ink/lib/diff.js:40:16)

Example file | Component file

Any ideas?

@ignu
Copy link

ignu commented Jul 8, 2017

I got this at one point as well... It took me a while to figure out the right babel settings for this project. This .babelrc is working for me in a sample app.

{
  "presets": ["babel-preset-react"],
  "env": {
    "development": {
      "plugins": [
        [
          "transform-react-jsx",
          {
            "pragma": "h"
          }
        ]
      ]
    }
  }
}

@brigand
Copy link
Contributor Author

brigand commented Jul 8, 2017

Doh, wasn't exporting the component. I'll add a warning for that in a separate PR.

@brigand
Copy link
Contributor Author

brigand commented Jul 8, 2017

@vadimdemedes I think this is ready to merge if you want it.

image

To try it: npm install, ./examples/run progress. Also see the docs.

const max = Math.min(Math.floor(space * percent), space);
for (let i = 0; i < max; i++) {
str += char;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

char.repeat(max);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

index.js Outdated
@@ -19,6 +20,7 @@ exports.Newline = Newline;
exports.Indent = Indent;
exports.Group = Group;
exports.Text = Text;
exports.Bar = Bar;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just Bar is too ambiguous. ProgressBar would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

/* @jsx h */
const {h, mount, Component, Text, Bar} = require('../');

const MAX = 20;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Max what? Would be nice if the variable name was more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried a different example.

}

render() {
const text = `Running `;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just inline this. Everything else is inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in two places, should I hard code the length?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, didn't notice that. Would be nice if the bar just calculated the left thing automatically. Feels weird having to manually specify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd have to know what else is on the line, or take the other content as a prop and render it itself, and it couldn't be custom components, and it might be a with multiple to have different colors.

It could have you either pass leftText (string) or leftSize (number), and if you pass leftText it'd render it. This seems confusing though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Ink should handle that for us. That's the behavior I expected. That would mean Ink would provide us the available screen space instead of us using process.stdout.columns directly. Let's wait and see what @vadimdemedes thinks.

Copy link
Owner

Choose a reason for hiding this comment

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

Very good idea, I'd want that to be implemented natively too. Although I've been thinking and didn't come up with a way to solve this properly. Opened #5 to discuss this.

.babelrc Outdated
]
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put the Babel config in package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

docs/Bar.md Outdated
@@ -0,0 +1,40 @@
The Bar component represents progress. See examples/progress.js for an example app.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a title: # Bar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. ProgressBar.


## Props

### char
Copy link
Collaborator

Choose a reason for hiding this comment

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

char => character

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@sindresorhus
Copy link
Collaborator

Nice work @brigand. This is looking very good :)

@vadimdemedes
Copy link
Owner

vadimdemedes commented Jul 9, 2017

@sindresorhus Thank you very much for reviewing and helping!

@brigand Thanks to you as well for writing a great component. I'd love it, if you could release it as an individual project (ink-progress-bar). My goal is to keep the core very minimal and contain the most basic set of required components, such as: <Text> (still thinking if I should extract it), <Indent>, <br> and <div>. With that strategy, Ink remains small, but developers can pick & use additional components they specifically need. I hope I explained correctly, why I'd rather release it separately instead of merging here.

I want to have a list of useful Ink components in the readme, like Preact's (https://github.com/developit/preact#libraries--add-ons), and I'd be more than happy to include yours there :)

@brigand
Copy link
Contributor Author

brigand commented Jul 9, 2017

Cool, done.

@brigand brigand closed this Jul 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants