Skip to content

Fix close button issue#481

Merged
stefanoborini merged 12 commits into
masterfrom
fix_stop_button_issue
Jun 5, 2017
Merged

Fix close button issue#481
stefanoborini merged 12 commits into
masterfrom
fix_stop_button_issue

Conversation

@martinRenou

@martinRenou martinRenou commented May 31, 2017

Copy link
Copy Markdown
Member

As it's not a "button" element but a "li" element, there is no disable attribute, we only play with the disabled class which change the visual aspect but doesn't prevent the click event to be triggered.

I fixed this by changing the disabled class which now prevents the mouse events.

Closes #479

@codecov-io

codecov-io commented May 31, 2017

Copy link
Copy Markdown

Codecov Report

Merging #481 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #481   +/-   ##
=======================================
  Coverage   95.24%   95.24%           
=======================================
  Files          88       88           
  Lines        3952     3952           
  Branches      247      247           
=======================================
  Hits         3764     3764           
  Misses        137      137           
  Partials       51       51

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 c6a3acb...e27d724. Read the comment docs.

As it's not a "button" element but an "li" element, there is no disable attribute, we only play with the disabled class which change the visual effect but doesn't prevent the click event to be triggered.
So the click event is still triggered and we have to deal with the case when the application is not currently running.
@stefanoborini

Copy link
Copy Markdown
Contributor

The solution is good, but I was wondering if we can actually prevent the click. Just curious.

@martinRenou

Copy link
Copy Markdown
Member Author

I changed it, it now prevents the click

@stefanoborini

Copy link
Copy Markdown
Contributor

No, leave the check. It makes sense it's there. You can't operate if it's not running, but it should also prevent the click.

@martinRenou

Copy link
Copy Markdown
Member Author

Ok, so I should do the same for "StartApplication" and add a check in order to stay consistent

@stefanoborini

Copy link
Copy Markdown
Contributor

Please add a test that the thing is actually not starting or stopping when those conditions happen.

@stefanoborini stefanoborini self-requested a review June 5, 2017 09:50

@stefanoborini stefanoborini left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Needs tests for the new code added

@stefanoborini stefanoborini merged commit 5f85e2a into master Jun 5, 2017
@stefanoborini stefanoborini deleted the fix_stop_button_issue branch June 5, 2017 11:59
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.

Stop application can be clicked and triggers red dot status indicator

3 participants