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

Maker: Setup page improvements and initialization cleanup #13286

Merged
merged 7 commits into from Feb 17, 2017

Conversation

@islemaster
Copy link
Member

islemaster commented Feb 16, 2017

ezgif com-optimize

Takes advantage of the stateless board reset introduced in #13252 to remove some now-dead code and allow re-detect on the setup page without a page reload. Also made some cosmetic changes to the setup page 'detect' sequence that make it feel more reliable.

  1. Re-detect is disabled during board detection, just in case there are weird edge cases for trying to restart detecting while it's in progress.

  2. I've added a tiny artificial delay to several setup steps - I think it just feels better if you see the spinner for a moment on each step before it turns green, even though several of these can be done near-instantly.

  3. We now animate the LEDs on successful connection instead of just turning them all on and off again. I think it looks nicer. It also seems to work around an issue where the board was dropping some messages, causing us to miss LEDs or skip notes in the fanfare. There's still a deeper investigation into the dropped messages issue going on, but I think making the setup page behavior more consistent is a good first step.

islemaster added 4 commits Feb 16, 2017
1. The re-detect button no longer triggers a setup page reload, because we're now able to initialize from scratch multiple times on a page.

2. Re-detect is disabled during board detection, to avoid any confusion.

3. I've introduced some artifical delay into the setup steps - it's more reassuring and satisfying to see each step spin for a moment before turning green, than to have most steps succeed instantaneously.

4. Issues with dropped serialport traffic meant we sometimes didn't manage to enable or disable all the LEDs at once on successful connection.  I've made success animate the LEDs in sequence instead, which is not only more dynamic and interesting, it seems to be much more reliable.  We'll address general issues with dropped messages soon, but for now making the setup page seem more reliable will help.
return BoardController.getDevicePort().then(port => this.connectToBoard(port));
static connect() {
return BoardController.getDevicePortName()
.then(portName => BoardController.connectToBoard(portName));

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

Nit: Now that connectToBoard is a static, I think we could just do BoardController.getDevicePortName().then(BoardController.connectToBoard)

this.setState({...this.getInitialState(), isDetecting: true});

if (!isChrome()) {
this.fail('IsChrome');

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

It looks like there is a discrete set of allowing things that can pass fail. Should we have some sort of enum, such that we end up doing this.fail(BoardStatus.IsChrome)?


// Put the board back in its original state, if possible
.then(() => bc.reset())
.then(() => this.setState({isDetecting: false}));

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

Can you imagine doing all this without arrow functions?

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 16, 2017

Author Member

No, no I can't.

* @param {BoardController} bc
* @returns {Promise} resolved when the song and animation are done.
*/
celebrateAllSuccessful(bc) {

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

Haha, love the method name. 🎆

clearInterval(interval);
resolve();
}
}, delay);

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

A bit of a nit, but I wonder if this would be more clearly expressed as a series of setTimeout's instead? (Might not really matter, feel free to ignore).

This comment has been minimized.

Copy link
@islemaster

islemaster Feb 16, 2017

Author Member

Oh, more like leds.forEach((led, i) => setTimeout(() => cb(led), i * delay)); - I could see that. I'll give it a shot.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Feb 16, 2017

Contributor

That might be even better than what I was thinking :-P

I had just been thinking of instead of doing a setInterval and clearing on the last index, do a setTimeout, and from within the function, call yourself again on a timeout unless it's the last index. Probably no better than what you have currently.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Feb 16, 2017

Review feedback addressed.

@islemaster islemaster merged commit d495b10 into staging Feb 17, 2017
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!
@islemaster islemaster deleted the maker-initialization-refactor branch Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.