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

feat(app): Enable new connectivity card #2517

Closed
wants to merge 2 commits into from

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Oct 19, 2018

overview

This PR also serves as a ticket for applying component library styling the wifi dropdown (react-select).

Previous connectivity cards were labeled as refactors due to the fact that any new features were behind a feature flag. This PR also removes the old connectivity card in favor of the old one, which now allows for connecting to unsecure networks and WPA2 personal.

screen shot 2018-10-19 at 1 19 47 pm

screen shot 2018-10-19 at 1 19 58 pm

changelog

  • feat(app): Enable new connectivity card
  • refactor(app): Apply component styling to wifi dropdown
  • refactor(app): Move wifi connect modal to new SelectNetwork + remove old components

review requests

This is a little whacky. A combination of custom components to replace react-select defaults and styling with emotion was needed to match the designs. I'm open to revisiting this if we move this react-select dropdown into the components library.

  • Can successfully connect to unsecured network on select
  • Can successfully connect to WPA2 network via modal
    • Wifi connect success modal renders and refreshes robot and network lists
    • Wifi connect error renders error modal (only visible when connected via USB)

TODO

Should we hold off on merging this until validation is present in the form? Or at least a disabled submit button when password is empty?

@Kadee80 Kadee80 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review small labels Oct 19, 2018
@Kadee80 Kadee80 self-assigned this Oct 19, 2018
@Kadee80 Kadee80 requested a review from mcous October 19, 2018 17:22
@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #2517 into edge will increase coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #2517      +/-   ##
==========================================
+ Coverage   50.69%   50.77%   +0.08%     
==========================================
  Files         626      625       -1     
  Lines       16086    16058      -28     
==========================================
- Hits         8155     8154       -1     
+ Misses       7931     7904      -27
Impacted Files Coverage Δ
app/src/config/index.js 55.55% <ø> (ø) ⬆️
app/src/components/RobotSettings/index.js 0% <ø> (ø) ⬆️
app/src/components/RobotSettings/ConnectionCard.js 0% <ø> (ø) ⬆️
app/src/components/RobotSettings/connection.js 0% <0%> (ø) ⬆️
api/src/opentrons/util/environment.py 90.24% <0%> (-2.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 224cf8d...893c4d9. Read the comment docs.

{SUCCESS_MESSAGE}
</AlertModal>
</Portal>
<AlertModal
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the WifiConnectModal into SelectNetwork folder

@Kadee80 Kadee80 added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Oct 22, 2018
@Kadee80
Copy link
Contributor Author

Kadee80 commented Oct 22, 2018

Removing the feature flag at this time would break backwards compatibility at this time. Closing this PR in favor of a smaller styling specific PR #2522

@Kadee80 Kadee80 closed this Oct 22, 2018
@Kadee80 Kadee80 deleted the app_enable-new-connection-card branch November 2, 2018 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available feature Ticket is a feature request / PR introduces a feature medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant