[ATL-1107][ATL-1045] Verify/Upload Buttons Hover & "running" style #197
Conversation
fstasi
changed the title
| // even with buttons disabled, better to double check if an upload is already in progress | ||
| if (this.uploadInProgress) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I added this check in order to future-proof the verify-sketch implementation:
https://arduino.atlassian.net/browse/ATL-1045 reports an error (Error: Request upload failed with message: 2 UNKNOWN: exit status 1) when a verify/build is run while another is in progress.
At the moment we only have buttons/menu items, but the check whether or not continue with the action should be inside the action handler itself in my opinion..
Do you have concerns about this approach?
There was a problem hiding this comment.
but the check whether or not continue with the action should be inside the action handler itself, in my opinion.
It is there; you added the isEnabled method: https://github.com/arduino/arduino-ide/pull/197/files#diff-5c246e36e23a59755c9cc7d5a116eac5be34d188c0eb681f8c449915cbfe9aacR30.
In theory, if the command handler is not enabled, the command should not run. I am fine with this approach. I just wanted to know if you did hit an error or why did you add it.
| id: 'arduino.toolbar.toggleBackground', | ||
| defaults: { | ||
| dark: 'editor.selectionBackground', | ||
| light: 'editor.selectionBackground', | ||
| hc: 'activityBar.inactiveForeground' | ||
| }, | ||
| description: 'Toggle color of the toolbar items when they are currently toggled (the command is in progress)' |
There was a problem hiding this comment.
@kittaakos this approach should be valid:
I created a new variable associated to existing theme colors. This way I will be able to have a semantic meaning for the color, while relying on the themes.
Works for you?
|
@fstasi, if the PR is ready for review, please quash the commits, and I look into it. Thank you! |
fstasi
force-pushed
the
fstasi/verify-button
branch
from
0bf73a1 to
a963e32
Compare
@kittaakos squashed and ready for review :) |
|
I have a few remarks on the behavior:
IDE2:
|
|
I have restarted the build, it was failing due to some issues out of the context of the project: HTTP 5xx is a server error, there isn't much we could do. Hopefully, we get back the green build 🤞 |
|
There is no visual indication of the toolbar items when hovering over them or running the corresponding command with the HC theme.
|
fstasi
force-pushed
the
fstasi/verify-button
branch
2 times, most recently
from
722d79d to
dd6a039
Compare
fstasi
changed the title
There was a problem hiding this comment.
the behaviour is correct, but disabled buttons (such as the debug button) should not receive a hover when they are disabled.
steps to reproduce:
- open a sketch
- select Arduino UNO (does not support debugging)
- ensure the button is greyed out
- hover
- verify it changes colour
fix hover state on toolbar items Improved statemanagement for ToolbarItem and Menus Disable Upload buttons while a sketch upload is already in progress toggled state to have override disabled button opacity doublecheck internal status before verify/upload a sketch fixes after code review
fstasi
force-pushed
the
fstasi/verify-button
branch
from
dd6a039 to
56fb68f
Compare
@ubidefeo fixed, thanks for pointing that out. I'm glad I had a spare very old arduino uno to test it 🧪 let me know if we are good to go |
fstasi
changed the title
per1234
added
topic: code
Creating another PR as I accidentally merged #190
This PR is an amend to the previous one, containing fixes after @kittaakos review
I Will squash merge this PR into main once the review is over, in order to have a single commit on the main branch