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

Settings/SelectBox: New option descriptions not calculated for contextview margins & overflow #57447

Closed
cleidigh opened this issue Aug 28, 2018 · 22 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release dropdown DropDown (SelectBox widget) native and custom issues settings-editor VS Code settings editor issues verified Verification succeeded
Milestone

Comments

@cleidigh
Copy link
Contributor

cleidigh commented Aug 28, 2018

Ref: #57304

The SelectBox custom drop-down calculates and manages margins for both bottom and top. It ensures
that there is always a margin above the status bar. It's

The new option extended descriptions added into the drop-down do not get included in the calculations
and therefore the drop down can overflow into the status bar.

@JacksonKearl - let me know if you want to take this or have me pick it up.

image

This screenshot shows a related issue: probably a calculation issue for a number of options to show -

image

@cleidigh cleidigh changed the title Settings/SelectBox: Option descriptions not calculated for contextview margins Settings/SelectBox: New option descriptions not calculated for contextview margins & overflow Aug 28, 2018
@cleidigh cleidigh self-assigned this Aug 28, 2018
@cleidigh cleidigh added bug Issue identified by VS Code Team member as probable bug settings-editor VS Code settings editor issues dropdown DropDown (SelectBox widget) native and custom issues labels Aug 28, 2018
@roblourens
Copy link
Member

@JacksonKearl 's internship is complete so it's yours if you want it 👍

@cleidigh
Copy link
Contributor Author

@roblourens
wait a minute he can't leave until he's done ;-)

But I'll take it

@roblourens roblourens added this to the August 2018 milestone Aug 28, 2018
@cleidigh
Copy link
Contributor Author

@roblourens
The drop-down height calculation adjustment got removed 👎
can I be assigned as reviewer for future drop-down stuff? I would've passed on calculation requirement had I known.

@roblourens
Copy link
Member

Sorry, that's my fault. I'll make sure to loop you in on other dropdown changes.

@cleidigh
Copy link
Contributor Author

absolutely no sorry 's ! I was just surprised it broke.
I think there are auto assignments for issues and PR 's, can I get added that way?

@roblourens
Copy link
Member

@chrmarti do we have enough issues with the dropdown label to train the bot on it?

@cleidigh
Copy link
Contributor Author

I assume this '_sticky' was meant to be removed?
looks like it maintains the drop-down open onBlur where it normally closes

	private _dropDownPosition: AnchorPosition;
	private detailsProvider: (index: number) => { details: string, isMarkdown: boolean };
	private selectionDetailsPane: HTMLElement;


	private _sticky: boolean = false; // for dev purposes only

	constructor(options: string[], selected: number, contextViewProvider: IContextViewProvider, styles: ISelectBoxStyles, selectBoxOptions?: ISelectBoxOptions) {

		this.toDispose = [];
		this._isVisible = false;
		this.selectBoxOptions = selectBoxOptions || Object.create(null);

@roblourens
Copy link
Member

I think we can leave it, it's useful for development. The suggest widget has the same thing, which is probably what inspired it.

@cleidigh
Copy link
Contributor Author

cleidigh commented Aug 29, 2018

@roblourens
Excuse me for bugging you, to fix this I have to understand a couple of things/your opinion:

Do you always want the extended descriptions at the bottom of the list, including when we flip the list above?

Do you have any maximum number of lines expected within the extended description? I have to contemplate how to calculate the space required for differing extended description heights. I will probably have to flip up based on the longest description for the list.

@roblourens
Copy link
Member

roblourens commented Aug 29, 2018

No problem at all.

Do you always want the extended descriptions at the bottom of the list, including when we flip the list above?

Yeah I think so... if putting them at the top is easier, that would work for me too.

Do you have any maximum number of lines expected within the extended description?

Since these can be localized and come from an extension, they could be any length. Basing it off the longest description is fine. Or, give it some reasonably large maximum, like 10 lines, and just truncate after that.

@cleidigh
Copy link
Contributor Author

okay , unfortunately since the new expressions completely ignore calculations to change layout I have to figure out whatever compromise to determine flipping for the layout. I will come up with a first pass and run it by you.

just as an aside, I'd like to mention it's a pleasure working on this stuff with you, VSC is pretty important to me and a great outlet, most importantly it's great to work with you guys.

@chrmarti
Copy link
Collaborator

@chrmarti do we have enough issues with the dropdown label to train the bot on it?

I included it in the last training, but it dropped out at the stage that checks the precision of the predictions for each label. I'll keep it in and eventually it will make the threshold.

@roblourens
Copy link
Member

Thanks @chrmarti!

@cleidigh It's a pleasure working with you too, and I appreciate your help with the settings editor!

@cleidigh
Copy link
Contributor Author

@roblourens
just want to give you update, proving to be a challenge , been working most of the day on this
hopefully I can get it right tomorrow...

@bpasero bpasero added the candidate Issue identified as probable candidate for fixing in the next release label Aug 30, 2018
@roblourens
Copy link
Member

Hey @cleidigh how is this going? What can I do to help? Do you think we'll be able to get it in today or tomorrow?

@cleidigh
Copy link
Contributor Author

Hi @roblourens
was just about to ping you about milestone protocol. I finally figured out what I think is a workable solution, not my favorite, but I think we can improve later necessary. I had to figure out a way
to create a correctly rendered description block that mirrors the description for each option. I have the same problem originally trying to measure for the list, you can't measure what's not rendered and in the DOM . there are some strange CSS inheritance which was causing me heartache. I should be able to patch in the last logic and then cleanup. If I don't get it in tonight tomorrow is still okay? If I run into Sunday is that a problem?

@roblourens
Copy link
Member

I'd prefer today, tomorrow is ok, but we don't want to take any big changes after tomorrow. I can help debug if you can open a PR with what you have. I don't have any other settings issues I'm working on right now.

@cleidigh
Copy link
Contributor Author

cleidigh commented Aug 30, 2018

okay one way or the other I will post something tonight and give you a progress report.
I think I'm past the big problems.

@cleidigh
Copy link
Contributor Author

cleidigh commented Aug 31, 2018

@roblourens
here's a problem with the current approach -

  • if we have to flip above due to lack of available space , if the user scrolls and the size of the description changes, the context for a container anchor point does not change (no current way to change)
    with the dynamic content at the bottom of the list but above the select, an increase in the description size overflows into the select below. If we could change the anchor point and grow the container
    there would not be overflow but the container would jump up and down as the user changed selection.

I think we might need to consider two options, dynamic content at top or scroll the select up and always open to the bottom.

image

image

image

@cleidigh
Copy link
Contributor Author

@roblourens
see WIP PR #57653

@roblourens
Copy link
Member

roblourens commented Aug 31, 2018

Thanks for the PR @cleidigh

Since we are at the end of endgame week and only want to take fixes for very serious issues, I propose that we take a simpler fix for the glitchy behavior of some select boxes, and continue improving the layout of the select box with details next month.

I pushed a simple fix that should restore the sizing of existing select boxes outside of the settings editor - I didn't realize this affected them. And it should make the select box + details in the settings editor look fine in most cases. It's still possible to get it to overlap the status bar or detach from its anchor element but I realize we need a more sophisticated fix like in your PR...

Verification

  • Try custom dropdowns on Windows in the terminal, output panel, debug viewlet, anywhere else they show up. Ensure they have the correct height and don't have weird empty spaces
  • Try dropdowns in the settings editor, for different settings with or without enumDescriptions, and change the scroll position of the settings list and the height of the window.

@roblourens
Copy link
Member

roblourens commented Aug 31, 2018

Opened a new issue for the continuing work...

#57662

roblourens added a commit that referenced this issue Aug 31, 2018
@bpasero bpasero added the verified Verification succeeded label Aug 31, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug candidate Issue identified as probable candidate for fixing in the next release dropdown DropDown (SelectBox widget) native and custom issues settings-editor VS Code settings editor issues verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants