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

[micro:bit] Setup Checklist - Remove 4th Check #44954

Merged
merged 7 commits into from Mar 2, 2022
Merged

Conversation

epeach
Copy link

@epeach epeach commented Feb 22, 2022

The previous code here wasn't catching connection errors. As we move closer to a Beta release, I wanted to loop back and make sure we were handling connection errors appropriately.

This change adds a catch to the 'connect' process for the micro:bit to catch and throw errors when needed. I tested locally by forcing the connection to fail (returning a failed promise in MBFirmataWrapper).

This PR also includes changes to add an additional check at the third step for the micro:bit. This check requests the firmware version from the micro:bit. The value we expect back includes markers that can help us determine if the board has outdated firmware on it. If the firmware version doesn't match our expectations, we fail the step and the user is served information about flashing the correct Firmata to their board.

Finally, this board removes the 4th step, see gif. This fourth step is relevant to the set-up for the Circuit Playground and doesn't have an analogous step for micro:bit. The expected behavior is that once a micro:bit is detected, we no longer render the fourth step and the third step now displays the thumbs-up icon when the process is complete.

Before:
Screenshot from 2022-02-21 15-45-56

After:
ThreeSteps

After discussing in the comments below, we are updating the final outcome so that 'waiting' steps aren't shown. See gifs.
4stepLoad

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@epeach epeach requested a review from a team February 22, 2022 00:55
{this.installFirmwareSketch()}
{this.contactSupport()}
</ValidationStep>
{this.state.boardTypeDetected !== BOARD_TYPE.MICROBIT && (
Copy link
Contributor

Choose a reason for hiding this comment

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

it feels a little confusing to have this step render then disappear once we know it's a micro:bit. could we invert this logic and only show this step for boards that should have it? alternatively, we might be able to check that boardTypeDetected && boardTypeDetected !== BOARD_TYPE.MICROBIT to get the same outcome

Copy link
Author

Choose a reason for hiding this comment

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

@KylieModen - I'd love your input here. I think Maddie's point makes sense, but that would change the current behavior for CP, vs. this change which keeps the current UI the same for CP, but changes for MB. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the flashing feels weird - I also don't love the order changing for what gets the thumbs up

Is there any other way to not display anything initially to avoid the flash? that way CP users see what they always have, and MB users will only see their UX?

Copy link
Author

@epeach epeach Feb 26, 2022

Choose a reason for hiding this comment

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

That's a tough one - the board is detected at the second step - so we don't know whether we need 3 or 4 steps until them. So we could display only each step when it's completed and not the "pending" grey step? So the behavior for CP would change a little bit, but each group would only see the steps applicable to them.

Copy link
Author

Choose a reason for hiding this comment

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

Mocked it up so we could see what that might look like:
4stepLoad
3stepload

Copy link
Contributor

Choose a reason for hiding this comment

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

love this update, LGTM

.then(serialPort => this.boardClient_.connectBoard(serialPort))
.then(() => {
if (
this.boardClient_.firmwareVersion.includes('micro:bit Firmata 1.1')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 'micro:bit Firmata 1.1' should probably be a constant at the top of the file for easy visibility -- it feels a little hidden here

@epeach
Copy link
Author

epeach commented Feb 28, 2022

@madelynkasula - PTAL! Thanks!

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

🎉 great work, erin!

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

3 participants