Add Toggle Output Scroll Lock action #18768

Merged
merged 7 commits into from Jan 24, 2017

Projects

None yet

4 participants

@ArtemGovorov
Contributor

As discussed here #18589 (comment).
@isidorn I have implemented the action and added it to the panel, but it's missing the icon. I'm also not sure how to make the action button reflect if the output is locked or not. Perhaps it's possible to use the action checked property and style the checked button in the same CSS file where the icon will be referenced, but the issue is that the action is not channel specific (ie there's only one instance of the action per panel, not per channel).

@ArtemGovorov ArtemGovorov Add Toggle Output Scroll Lock action
5a05170
@msftclas

Hi @ArtemGovorov, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@ArtemGovorov ArtemGovorov Make Toggle Output Scroll Lock action change its state based on a sel…
…ected channel
ceec3da
@ArtemGovorov
Contributor

but the issue is that the action is not channel specific (ie there's only one instance of the action per panel, not per channel).

Ok, so I found how to make the action checked state to consider the current active channel, added the commit to support it.

So what's left now is an icon and some CSS for the action-label to display the desired checked state (like a border or a separate background).

@isidorn isidorn was assigned by dbaeumer Jan 19, 2017
@@ -111,6 +111,11 @@ export interface IOutputChannel {
output: string;
/**
+ * Returns the value indicating whether the channel has scroll locked.
+ */
+ hasScrollLock: boolean;
@isidorn
isidorn Jan 19, 2017 Contributor

I would rename this to scrollLock, as outputChannel.hasScrollLock = false sounds strange to me

+ }
+
+ public run(): TPromise<any> {
+ const activeChannel = this.outputService.getActiveChannel();
@isidorn
isidorn Jan 19, 2017 Contributor

What if there is no active output channel (output hidden) and this command is triggered from the command pallete?
You either need a null check here or even better do not register this action as a global workbench action as I do not see people assigning shortcuts to this action or triggering it via command pallete.

@isidorn
Contributor
isidorn commented Jan 19, 2017

@ArtemGovorov PR looks good overall. Added 2 comments which you can see directly in the commits.
You could play around with the css and just use a temporary fake icon. For example this one
https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/preferences/browser/media/collapseAll.svg#L1

Once @bgashler1 provides a lock icon I can just replace it

ArtemGovorov added some commits Jan 19, 2017
@ArtemGovorov ArtemGovorov Renamed `hasScrollLock` to `scrollLock` a6e63a1
@ArtemGovorov ArtemGovorov Removed workbench registration for the ToggleOutputScrollLockAction a…
…ction
827a287
@ArtemGovorov
Contributor

@isidorn Thanks for reviewing it. I have fixed both comments.

@isidorn isidorn added this to the January 2017 milestone Jan 19, 2017
@isidorn
Contributor
isidorn commented Jan 19, 2017 edited

@ArtemGovorov great! Let's wait for the icon before we merge this in.
In the meantime feel free to experiment with css and a fake icon as I mentioned above.

There seems to be build errors as our travis build is falling, can you also please look into that? As far as I see it seems like the Label and Id are needed on the action.

@ArtemGovorov ArtemGovorov Added back ID and LABEL for the ToggleOutputScrollLockAction action
1375b61
@ArtemGovorov
Contributor

There seems to be build errors as our travis build is falling, can you also please look into that? As far as I see it seems like the Label and Id are needed on the action.

Oops, my fault. Added these properties back.

In the meantime feel free to experiment with css and a fake icon as I mentioned above.

Cool, will be doing it now.

@ArtemGovorov ArtemGovorov Styled ToggleOutputScrollLock action
b4ce09a
@ArtemGovorov
Contributor

Added the styles for the checked state. As for the icon, foe now I just copy.pasted the Clear Output icon content, so all that's left is to replace the SVG in the 2 icons files (normal and inverted) with the new icon.

ctrb

@isidorn
Contributor
isidorn commented Jan 20, 2017

@ArtemGovorov looks great! Just one more minor thing please revert your changes to output.contribution.ts since our hygiene is failing since you introduced an unused import. In the end you should not have any changes in this file - thanks

@ArtemGovorov ArtemGovorov Removed unused import
4b90f08
@ArtemGovorov
Contributor

@isidorn Thanks, done.

@bgashler1
Member
bgashler1 commented Jan 23, 2017 edited

Here are a couple of different options we could go with.
scroll-lock-icons

And attached are the assets.
881665_Brad_VSCodeOutputWindowScrollLock_TwoIdeas.zip

Since we don't have a pattern today in our UX for state representation of icons (other than badging or swapping), my proposal would be to swap the icons out as shown above when the state changes.

@isidorn isidorn merged commit 9c9a615 into Microsoft:master Jan 24, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@isidorn isidorn referenced this pull request Jan 24, 2017
Closed

Test: output scroll lock #19138

2 of 2 tasks complete
@isidorn
Contributor
isidorn commented Jan 24, 2017

Created test plan item #19138
Took the first icon. Will not swtich between icons but will draw a blue border when the action is active as we do in the find widget. I find that less confusing for the user.
Choose the first icon because we never render the scroll in the manner that the second icon is showin, and the first one is more general

@ArtemGovorov
Contributor

Awesome, thanks! The icon looks great. So excited to see feature is being added to the next release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment