Skip to content
This repository was archived by the owner on Dec 16, 2021. It is now read-only.

Conversation

@delvedor
Copy link
Member

@delvedor delvedor commented Sep 4, 2016

Hi all, I've implemented the UI proposal as seen in #26.
I basically rewrited all the browser part.
Here's some screenshots:
screen shot 2016-09-04 at 3 53 47 pm
During installation:
screen shot 2016-09-04 at 3 46 08 pm

What do you think?
Still missing few things, such as the update of the code example and the error management if the installation fails.
For every question/suggestion I'm here.

If you want to try it by yourself clone this and run npm start.


- Update -
NOTE: the PR is not ready to be merged, I've opened early to get suggestions and feedback and allow you to try it. The final version will not have the testing code (see fakeInstall).

browser.js Outdated
const getInstalledVersion = require('./lib/check-node')
const loadVersions = require('./lib/load.js')
// const install = require('./lib/install')
// for testing purpose

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@mikeal
Copy link
Contributor

mikeal commented Sep 6, 2016

LOVE the look of this :)

@delvedor
Copy link
Member Author

delvedor commented Sep 14, 2016

Hi, I've added the error message if the installations fails, here's the screenshot.
What do you think?

screen shot 2016-09-14 at 9 25 08 am

The next step is add more code examples.

@delvedor
Copy link
Member Author

I was wondering if could be a good idea add a Report button in case of the installation fails, which takes the user to this repo issues.
What do you think?

@mikeal
Copy link
Contributor

mikeal commented Sep 17, 2016

I was wondering if could be a good idea add a Report button in case of the installation fails, which takes the user to this repo issues.

That's a great idea! But that shouldn't hold up this commit, let's get this merged as soon as you think it's ready :)

@delvedor
Copy link
Member Author

For me this first implementation of the UI is ready, I've removed the testing code and added another code example.

Later I will open an issue for the report button and another one for the install function, in my opinion the update function (the second argument) is useless, install should only do his job, everything else can be done in the callback.

If you think there are other things to change before merging, please let me know.

@JoeDoyle23
Copy link
Member

LGTM. Merging in.

@JoeDoyle23 JoeDoyle23 merged commit 31f9d86 into nodejs:master Sep 17, 2016
@delvedor delvedor deleted the installer-ui branch September 17, 2016 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants