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

#22622 #23211

Merged
merged 10 commits into from
Jun 14, 2017
Merged

#22622 #23211

merged 10 commits into from
Jun 14, 2017

Conversation

cristianhosu
Copy link
Contributor

Please validate the layout and the implementation.
Need help making the scroll work from the mouse wheel inside the .debug-hover-widget

@mention-bot
Copy link

@cristianhosu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @egamma and @bpasero to be potential reviewers.

@msftclas
Copy link

@cristianhosu,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@isidorn
Copy link
Contributor

isidorn commented Mar 27, 2017

@cristianhosu thank you for your PR. However we are currently in the endgame, which means we are wrapping up our March milestone. Due to that I will check this PR and provide feedback next week.

@isidorn isidorn added this to the April 2017 milestone Mar 27, 2017
@cristianhosu
Copy link
Contributor Author

@isidorn Hello, can you please check?
Thank you

@isidorn
Copy link
Contributor

isidorn commented Apr 12, 2017

@cristianhosu yes I plan to look into this today / tomorrow. Thanks for the reminder

@isidorn
Copy link
Contributor

isidorn commented Apr 13, 2017

@cristianhosu great work!
Why did you add the z-index - that does not seem needed? Everything else looks good

@joaomoreno how does the suggest widget steal the scrolling from the editor?

@cristianhosu
Copy link
Contributor Author

@isidorn Thank you
I removed the z-index. I added it as a test to try to put the debug panel on top of the underlying panel to make it scrollable when inside it.

@isidorn
Copy link
Contributor

isidorn commented Apr 24, 2017

@cristianhosu sorry for the slow response, last week I was on vacation.
I still see the z-index, are you sure you pushed the commit on top of this pr.

@isidorn
Copy link
Contributor

isidorn commented Apr 25, 2017

Pushing to May since we are now in endgame, will merge it in next week.
Sorry for the delay, there was some bad timing regarding my vacation

@isidorn isidorn modified the milestones: May 2017, April 2017 Apr 25, 2017
@cristianhosu
Copy link
Contributor Author

@isidorn I've pushed to the correct branch as far as i can tell, but AppVeyor and Travis CI have not completed. maybe this is the reason why it's not visible?

@isidorn
Copy link
Contributor

isidorn commented May 8, 2017

@cristianhosu now it is good. I will merge it in and look into the scrolling issue. Thanks

@isidorn
Copy link
Contributor

isidorn commented May 8, 2017

I have looked into the scrolling issue and to make this proper we need to use the DomScrollableElement class. This will also give us consistent look and feel across different OS.
I am looking into this...

@isidorn
Copy link
Contributor

isidorn commented May 22, 2017

I am running out of cycles for this milestone and will not have time to look into using DomScollableElement class. @cristianhosu are you intrested into looking into this? Using this class will give us consistent scroll bar design across vscode. Nowhere else do we use the native scroll bar and that is the main reason why this PR can not be merged in right now.

@cristianhosu
Copy link
Contributor Author

cristianhosu commented May 23, 2017 via email

@isidorn
Copy link
Contributor

isidorn commented May 23, 2017

@cristianhosu great, thanks!

@isidorn isidorn modified the milestones: June 2017, May 2017 May 23, 2017
@cristianhosu
Copy link
Contributor Author

cristianhosu commented May 26, 2017 via email

@isidorn
Copy link
Contributor

isidorn commented May 29, 2017

@cristianhosu thanks for looking into this further.
Yes, this is a common issue people hit and we should improve this. You can find details on how to fix this in my comment here

One solution #20623 (comment)
Second solution is here #23831 (comment)

@isidorn
Copy link
Contributor

isidorn commented Jun 6, 2017

@cristianhosu let me know if there is anything else I can help with

@cristianhosu
Copy link
Contributor Author

@isidorn I have finished. I've finally made time to investigate how to properly implement a DomScrollableElement and it worked.

@isidorn
Copy link
Contributor

isidorn commented Jun 12, 2017

@cristianhosu great! Feel free to push those commits once they are ready so I can review them.

@cristianhosu
Copy link
Contributor Author

@isidorn I have pushed. Sorry about that... i keep forgetting to do so, being used to TFS :)

@isidorn
Copy link
Contributor

isidorn commented Jun 14, 2017

@cristianhosu absolutetly great work! Thanks a lot!
Just one more tiny thing: can you please remove the ScrollbarVisibility auto options passed on line 66, those are the defaults and we should not specify them. Once you do that you will notice that one import you added is not necessery.

@isidorn isidorn merged commit 6a26c57 into microsoft:master Jun 14, 2017
@isidorn
Copy link
Contributor

isidorn commented Jun 14, 2017

Great work!

@cristianhosu
Copy link
Contributor Author

@isidorn - thank you :)

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants