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

Status indicator #85

Merged
merged 4 commits into from
Mar 16, 2017
Merged

Status indicator #85

merged 4 commits into from
Mar 16, 2017

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Mar 15, 2017

Implements status indicator as per #40

- `isWatching` got deprecated in favor of `isBackground`.
- latest vscode does not recognizes version `0.3.2`.
- Adds status indicator inspired by one in vscode-jest, that is shown spinner when flow is type busy.
- Adds `flow.showStatus` setting so that status indicator can be disabled if not desired.
@orta
Copy link
Contributor

orta commented Mar 15, 2017

Any chance we can get a screenshot?

@orta
Copy link
Contributor

orta commented Mar 15, 2017

Other than that - everything looks 👍

@Gozala
Copy link
Contributor Author

Gozala commented Mar 15, 2017

Any chance we can get a screenshot?

status indicator

Note the spinner on the right side of the errors / warnings counter that appears on save.

@Gozala Gozala mentioned this pull request Mar 15, 2017
@orta
Copy link
Contributor

orta commented Mar 15, 2017

Alright, that looks good - one little addition, and this is good to go. This extension has an output channel, can you make clicking on the status toggle the visibility for it? Then people know where this output is coming from.

@Gozala
Copy link
Contributor Author

Gozala commented Mar 15, 2017

@orta latest change shows the output when indicator is clicked. It does not toggle though because from what I can tell there is no way of telling if the output channel is shown or not. Note that I can't just track that state on my own as there's no way of telling if initial state is shown or hidden. If you really want toggling I can go under assumption it's hidden initially, but in that case if output happened to be open, first click will do nothing.

P.S.: It also seems that showing output channel does not always work, not sure why. because vscode does not execute registered command on every click, no idea why.

@orta
Copy link
Contributor

orta commented Mar 16, 2017

interesting, I think just showing is reasonable. The worrying thing is the unreliablilty - perhaps it's worth adding "Flow: " beforehand then so that people know what it going on?

My guess is that the spinner might be re-creating the status bar item, and potentially the re-hooking of the click notifier happens a tick or two after, but the setInterval will reset it again before it's been added back?

@Gozala
Copy link
Contributor Author

Gozala commented Mar 16, 2017

interesting, I think just showing is reasonable. The worrying thing is the unreliablilty - perhaps it's worth adding "Flow: " beforehand then so that people know what it going on?

If you hover the status indicator tooltip will appear with a message: 'Flow is type checking', which I assumed was enough & realistically user might wonder what's that only first time around. That being said if you really want to I can add additional setting to allow prefxing spinner with arbitrary string with a default "Flow: ".

My guess is that the spinner might be re-creating the status bar item, and potentially the re-hooking of the click notifier happens a tick or two after, but the setInterval will reset it again before it's been added back?

Well the code I've written just updates text and visibility of the same statusbar item. That being said I don't know what vscode does under the hood, it could be that updating text does some of the rehooking as you described but find it hard to believe. Also I tried spinner that does not update text nor visibility & it did not seem to make any difference :(

@orta
Copy link
Contributor

orta commented Mar 16, 2017

Alright, that seems 👍 with me ( I had the same issues after jest-community/vscode-jest#63 was merged on another project - but haven't really found an answer )

The hover alt text should be fine. Let's do it.

@orta orta merged commit 6a0e872 into flow:master Mar 16, 2017
Gozala added a commit to Gozala/flow-for-vscode that referenced this pull request Mar 16, 2017
orta added a commit that referenced this pull request Mar 16, 2017
Describe changes from #85 #87 #88 in changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants