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 zoom button should be a IStatusbarEntry #74454

Closed
bpasero opened this issue May 28, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@bpasero
Copy link
Member

commented May 28, 2019

I noticed that ZoomStatusbarItem is a IStatusbarItem. This one is typically when you need full control over the rendering of the item, but it seems you are only using text. I suggest to change this to IStatusbarEntry and use IStatusbarService#addEntry() to show and hide it dynamically. You can also update properties dynamically on the returned object.

I had to push 4eb7879 because I noticed that some style changes broke the visual appearance of this item. We could avoid issues like these in the future by switching to the same model e.g. extensions are using too.

Is there anything that blocks us from going there? I could not see anything when I was checking the implementation quickly.

@mjbvz

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Thanks for the heads up. No blockers that I know of, I think I just copied what other places in the code were doing

@skprabhanjan

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@mjbvz , I can take a look at this, Will do initial analysis and let you know.
Any pointers from your side will also be helpful.

@mjbvz

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Start by looking at some of the other places we use IStatusbarEntry in the codebase, such as here or here to understand how to use the new API. Then see if you can convert the logic from the existing IStatusbarItem to use an IStatusbarEntry instead.

One good initiail approach would be to convert the methods of ZoomStatusbarItem such as show and hide to use an IStatusbarEntry internally

@bpasero bpasero changed the title Status zoom button could be a IStatusbarEntry? Status zoom button should be a IStatusbarEntry Jun 3, 2019

@bpasero

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

@mjbvz @skprabhanjan one potential issue I can forsee is that we seem to be opening a context menu from the position of the status bar element and with the other API you do not get any hold of that element.

However we can trick the API and simply put the id of the status bar entry as actual DOM ID to the status element (somewhere in statusbarPart.ts) and then use document.getElementById() with that ID to show the menu properly.

burknator added a commit to burknator/vscode that referenced this issue Jun 14, 2019

Makes status zoom button a IStatusbarEntry microsoft#74454
ZoomStatusbarItem can now be instantiated, which is done in
InlineImageView, which it uses to update the status bar.
The needed services are passed down via constructors,
starting in BaseBinaryResourceEditor.
@burknator

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@skprabhanjan Are you working on this and have a PR ready? Otherwise I would like to submit one.

@skprabhanjan

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@burknator , Please feel free to work on it.

burknator added a commit to burknator/vscode that referenced this issue Jun 17, 2019

Makes status zoom button a IStatusbarEntry microsoft#74454
ZoomStatusbarItem can now be instantiated, which is done in
InlineImageView, which it uses to update the status bar.
The needed services are passed down via constructors,
starting in BaseBinaryResourceEditor.

burknator added a commit to burknator/vscode that referenced this issue Jun 17, 2019

Makes status zoom button a IStatusbarEntry microsoft#74454
ZoomStatusbarItem can now be instantiated, which is done in
InlineImageView, which it uses to update the status bar.
The needed services are passed down via constructors,
starting in BaseBinaryResourceEditor.
Removes padding from CSS which was necessary because of previous
implementation.

@mjbvz mjbvz closed this in #75618 Jun 21, 2019

mjbvz added a commit that referenced this issue Jun 21, 2019

Merge pull request #75618 from burknator/fixes/image-zoom-button
Makes status zoom button a IStatusbarEntry #74454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.