-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Polish code editor and fix iPhone issue #22021
Conversation
Size Change: +69 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
This branch when switched to, for me at least, has very subtle differences from master. In the simulator and on the iPhone they're about the same. How can I encounter the poor perf situation on master? |
THANK YOU for testing and reviewing!
There are some visual changes to polish the code editor, and there's some JS that was written for the visual editor that breaks the code editor, that I've conditioned out. Both of these benefit the experience overall, and deserve their own thoughts if you have any. On "breaking the code editor", quite simply you shoudl be able to type a bunch of text in the simulator, this doesn't work on master virtually at all.
The truth is, I never encountered this myself. When I worked on the previous version of this branch, I thought that things were faster when the textfield was not "autosized" — but upon further examination this doesn't seem to be the case. So performance wise, and since I can't really reproduce, that aspect of this branch should technically be unchanged from master. |
Thanks for working on these changes, @jasmussen! I don't have any feedback on the iPhone issue, but I did catch this: The way the editor content drops behind the white bar can use some more work. I see why it does this when I made the screen size smaller. Those two text elements end up overlapping the editor. All the more reason I'd like to see those move somewhere else. But I like the approach of incremental changes and I'd love to see these PRs continue. So maybe we can try adjusting the opacity of that white just a bit (ie. |
Good call, Mark, and thank you for reviewing! The reason for the white is to work when you scroll, of course, and the reason for not having a bottom border is to try and reduce those overall — it feels like for every border we remove, we ligthen the weight a little bit. So your suggestion feels like a good path forward: |
If thinks look good, @mapk, I would appreciate a thumbs up! |
Golly, I specifically created this one to avoid that. Let me take another look. |
240d879
to
ace94b9
Compare
Do you have steps to reproduce? I can't get the same state when I test. Safari? |
Yes, safari :) and it doesn't happen until I focus the textarea (or classic editor) |
I still can't reproduce: Note that if you normally browse with auto-hiding scrollbars, and then turned them on to test, you need to restart the browser for their behavior to be accurate. Edit: just to be clear, the double scrollbar is supposed to be there in non-fullscreen mode when the left sidebar navigation has to scroll. |
Actually, I see now that I reproduce on master too, so it seems unrelated with this PR. |
If it's good to go, I could use a 👍 👍 :) |
This is an alternative to #21950. But it takes a different approach. The other PR was intended to fix an observed performance issue in Safari, but I haven't been able to fully reproduce it. Originally it felt like the auto-sizing textarea was the cause of the issue, but I'm not sure that's actually the case.
Additionally, removing the auto-sizing textarea has some compounding effects on the structure and layout. Immediately, it means the textarea has to be full-height, and scroll itself. That means the layout of the page should be flexed instead. It also means the still-autosizing title block needs a max-height. And then I discovered there were some issues with Mobile Safari and some of the JS we wrote to improve the visual aspects.
All of the issues above are fixed and addressed in #21950. However I still think this branch is the better one.
It seems worth comparing the two branches side by side, not only to get a feel for the changes, but to note any perceived Safari performance issues in either.
This branch goes a little further in that it also polishes the visuals a bit more:
Screens: