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

fix(app): Prevent wrong keypress combo from changing jog jump size #2315

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Sep 18, 2018

overview

This PR closes #2300. Clicking on a jog increment radio button caused an onFocus event which allowed an unused combination of our arrow keys + shift to change the jump size. The solution was to setState and then immediately .blur() the clicked element.

changelog

  • fix(app): Prevent wrong keypress combo from changing jog jump size

review requests

Try it out on VS or a Robot. Open up deck calibration or labware calibration (some screen with jog buttons)

  • expected key combos jog the robot or change the jog increment

Click on a radio button to choose a jog increment (focus the form element)

  • shift + left/right arrow keys does NOT change the jog increment
  • + or - keys still changes jog increment

@Kadee80 Kadee80 requested review from mcous and IanLondon September 18, 2018 19:47
@Kadee80 Kadee80 added bug app Affects the `app` project ready for review fix PR fixes a bug labels Sep 18, 2018
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.

Tested on Sunset:

  • expected key combos jog the robot or change the jog increment

Click on a radio button to choose a jog increment (focus the form element)

  • shift + left/right arrow keys does NOT change the jog increment
  • + or - keys still changes jog increment

⌨️

@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #2315 into edge will increase coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #2315      +/-   ##
==========================================
+ Coverage   30.16%   30.22%   +0.06%     
==========================================
  Files         502      502              
  Lines        8053     8169     +116     
==========================================
+ Hits         2429     2469      +40     
- Misses       5624     5700      +76
Impacted Files Coverage Δ
app/src/components/JogControls/index.js 0% <0%> (ø) ⬆️
app-shell/src/update.js 69.56% <0%> (-30.44%) ⬇️
app/src/components/AppSettings/index.js 0% <0%> (ø) ⬆️
app/src/components/AppSettings/AppUpdateModal.js 0% <0%> (ø) ⬆️
app/src/pages/More/AppSettings.js 0% <0%> (ø) ⬆️
app-shell/src/main.js 0% <0%> (ø) ⬆️
...src/components/AppSettings/AdvancedSettingsCard.js 0% <0%> (ø) ⬆️
app/src/components/AppSettings/AppInfoCard.js 0% <0%> (ø) ⬆️
app/src/shell/index.js 78.78% <0%> (+7.35%) ⬆️

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 02a404b...d44ee09. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Sep 18, 2018

Codecov Report

Merging #2315 into edge will increase coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #2315      +/-   ##
==========================================
+ Coverage   30.16%   30.22%   +0.06%     
==========================================
  Files         502      502              
  Lines        8053     8169     +116     
==========================================
+ Hits         2429     2469      +40     
- Misses       5624     5700      +76
Impacted Files Coverage Δ
app/src/components/JogControls/index.js 0% <0%> (ø) ⬆️
app-shell/src/update.js 69.56% <0%> (-30.44%) ⬇️
app/src/components/AppSettings/index.js 0% <0%> (ø) ⬆️
app/src/components/AppSettings/AppUpdateModal.js 0% <0%> (ø) ⬆️
app/src/pages/More/AppSettings.js 0% <0%> (ø) ⬆️
app-shell/src/main.js 0% <0%> (ø) ⬆️
...src/components/AppSettings/AdvancedSettingsCard.js 0% <0%> (ø) ⬆️
app/src/components/AppSettings/AppInfoCard.js 0% <0%> (ø) ⬆️
app/src/shell/index.js 78.78% <0%> (+7.35%) ⬆️

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 02a404b...d44ee09. Read the comment docs.

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

Tested on VS
⛳ 🥅

@Kadee80 Kadee80 merged commit 1b32d6d into edge Sep 19, 2018
@Kadee80 Kadee80 deleted the app_fix-jog-key-jump-size branch September 19, 2018 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project bug fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jump Size keyboard control in macOS during calibration
3 participants