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

refactor(app): Add ability to add/remove manual IP addresses #3256

Merged
merged 7 commits into from
Mar 26, 2019

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Mar 25, 2019

overview

This PR closes #2742 and adds the ability to add or remove manual IP addresses / hostnames via the advanced settings modal in app settings.

This PR does NOT:

  • check for added IPs in discovery within this modal to add the spinner/checkmark icons to IpList

2019-03-25 15 46 11

changelog

  • feat(app): Add ability to add/remove manual IP addresses
  • refactor(components): Update ButtonProps to receive optional buttonRef

review requests

  • comment out line 55 in app-shell/Makefile
  • make -C app dev OT_APP_DEV_INTERNAL__MANUAL_IP=1
  • make -C api dev
  • dev USB does not render in initial app robot list

  • Go to app settings page
  • Click [manage] button in advanced settings (Manually Add Robot Network Addresses)
  • Modal opens with instructions and empty input
  • [+] button is disabled until text is entered

  • enter localhost as text and click [+] to submit
  • localhost renders below in IpList as an IpListItem with [-] button
  • discovery.candidates array in config.json now contains 'localhost'

  • enter any random string in the AddManualIpForm above and submit [+]
  • that random string renders below in IpList as an IpListItem with [-] button
  • discovery.candidates array in config.json now contains that random string

  • Click [-] button to remove the random string from the list
  • Clicking [-] removes the appropriate IpItem from the IpList
  • discovery.candidates array in config.json no longer contains that random string

  • Navigate to Robot tab in app and click [refresh list]
  • Dev USB shows up in robots list
  • localhost appears in discovery.json

@Kadee80 Kadee80 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review labels Mar 25, 2019
@Kadee80 Kadee80 requested a review from a team March 25, 2019 19:53
@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #3256 into edge will increase coverage by 1.25%.
The diff coverage is 7.79%.

Impacted file tree graph

@@            Coverage Diff            @@
##             edge   #3256      +/-   ##
=========================================
+ Coverage   53.74%     55%   +1.25%     
=========================================
  Files         700     704       +4     
  Lines       20563   21985    +1422     
=========================================
+ Hits        11052   12093    +1041     
- Misses       9511    9892     +381
Impacted Files Coverage Δ
discovery-client/src/index.js 85.85% <0%> (-5.54%) ⬇️
...p/src/components/AppSettings/AddManualIp/IpList.js 0% <0%> (ø)
app/src/config/index.js 33.33% <0%> (-22.23%) ⬇️
...p/src/components/AppSettings/AddManualIp/IpItem.js 0% <0%> (ø)
.../src/components/AppSettings/AddManualIp/IpField.js 0% <0%> (ø)
app-shell/src/config.js 24% <0%> (-1%) ⬇️
...components/AppSettings/AddManualIp/ManualIpForm.js 0% <0%> (ø)
...pp/src/components/AppSettings/AddManualIp/index.js 0% <0%> (ø) ⬆️
components/src/buttons/Button.js 100% <100%> (ø) ⬆️
app-shell/src/discovery.js 84.61% <50%> (-2.89%) ⬇️
... and 41 more

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 7d09d0a...0b3cc88. Read the comment docs.

Copy link
Contributor

@btmorr btmorr left a comment

Choose a reason for hiding this comment

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

🌔 Looks great. Tested locally.

@Kadee80
Copy link
Contributor Author

Kadee80 commented Mar 26, 2019

After chatting with the FE team, refs in buttons raises more issues than it solves. Going to strip out refs in buttons in favor of a pure CSS solution to the .blur() issue it solved. PR is now a WIP

- clean up disabled logic for submit button
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

One comment but otherwise good to merge 💵

@import '@opentrons/components';

:root {
--ip-group: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused set can be removed

@Kadee80 Kadee80 changed the title feat(app): Add ability to add/remove manual IP addresses refactor(app): Add ability to add/remove manual IP addresses Mar 26, 2019
@Kadee80 Kadee80 merged commit 9861463 into edge Mar 26, 2019
@Kadee80 Kadee80 deleted the app_add-manual-ip branch March 26, 2019 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manual IP Address: Add and Remove Network Address
3 participants