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.

Sign up for GitHub

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

Jump to bottom

[mobile] Prevent the IDE from scrolling along with the text (e.g. on iPad) #5742

Merged
merged 1 commit into from
Jul 24, 2019
Merged

[mobile] Prevent the IDE from scrolling along with the text (e.g. on iPad) #5742

merged 1 commit into from
Jul 24, 2019

Conversation

jankeromnes
Copy link
Member

Part of #3557 (thanks a lot @EricRabil for the tip! 💯)

Screencasts

Current scrolling behavior (IDE scrolls with code editor): https://youtu.be/T0So5v0GQBA

New scrolling behavior (fixed IDE): https://youtu.be/Q3oURvM4Dh0

@akosyakov akosyakov added ipad issues related to iPad ui/ux issues related to user interface / user experience labels Jul 19, 2019
@akosyakov
Copy link
Member

I don't have iPad to verify, but we should at least check that absolute -> fixed does not break anything. Maybe someone has an idea why we had absolute in the first place, after 2 years it is hard to remember 🤔

akosyakov
akosyakov approved these changes Jul 19, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can proceed with it. It will only break if Theia is embedded not as a iframe, but attached to some child element. Which is not supported right now.

@akosyakov
Copy link
Member

It would be good to get one more opinion before merging. At least that other agree, will merge it on Monday if no objections.

akosyakov
akosyakov previously requested changes Jul 19, 2019
View reviewed changes
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah no, it broke the overlay widgets. They are displayed in the wrong position now. Try to open any quick palette.

@jankeromnes
Copy link
Member Author

Thanks for the review!

I believe that the main difference between fixed and absolute is that fixed is positioned relatively to the viewport, while absolute is positioned inside the closest position: relative element.

I noticed that changing body to position: fixed broke the loading indicator, which I fixed by changing its position from absolute to fixed.

It's totally possible that my PR other absolutely positioned elements downstream. I'll try to fix the overlay widgets you mentioned (thanks for catching these!)

@akosyakov
Copy link
Member

@jankeromnes the main issue with this PR that in the future it won't allow to append Theia shell somewhere in the page, not to the body. I wonder is there a way to avoid switching to fixed.

@lmcbout
Copy link
Contributor

lmcbout commented Jul 19, 2019

Nice to have Theia working on a tablet.
Playing with Theia on Ubuntu and Chrome (I don't have a IPAD to test :( )
First thing I see is the quick palette, It shows offset to the left and unable to see the data. Even resizing the browser to full screen, the data is display off to the left.

quickPalettePosition

@vince-fugnitto vince-fugnitto mentioned this pull request Jul 19, 2019
Closed
@jankeromnes jankeromnes force-pushed the mobile-fixes branch 2 times, most recently from c7cc6bf to 13faed3 Compare July 22, 2019 14:27
@jankeromnes
Copy link
Member Author

Switched to @vince-fugnitto's touchmove approach from #5759 -- nice & simple, and works like a charm!

@jankeromnes
Copy link
Member Author

(Not sure what eclipsefdn/eca is complaining about, but if it's related to the Co-authored-by annotation, I can remove it if that's what it expects.)

@akosyakov akosyakov dismissed their stale review July 22, 2019 15:05

by new changes

@vince-fugnitto
Copy link
Member

I think the issue is the co-authored-by, you can see that the following PR doesn't have the issue.
I think it's fine excluding the co-authored-by :)

@jankeromnes
Copy link
Member Author

(eclipsefdn/eca negociation attempt: removed one Signed-off-by but kept the Co-authored-by.)

@jankeromnes
Copy link
Member Author

All green! @vince-fugnitto or @akosyakov please take another look when you have time. 😄

vince-fugnitto
vince-fugnitto approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me 👍
The bug regarding the previous iteration with the quick-open menu is no longer present either.

@akosyakov akosyakov merged commit 8114667 into eclipse-theia:master Jul 24, 2019
@akosyakov akosyakov deleted the mobile-fixes branch July 24, 2019 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@akosyakov akosyakov akosyakov left review comments

@lmcbout lmcbout Awaiting requested review from lmcbout

@kittaakos kittaakos Awaiting requested review from kittaakos

Assignees
No one assigned
Labels
ipad issues related to iPad ui/ux issues related to user interface / user experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jankeromnes @akosyakov @lmcbout @vince-fugnitto