-
Notifications
You must be signed in to change notification settings - Fork 28
Conversation
Do you want me to squash the last one into the others? |
Yes please! I will review this in just a bit. |
Done. |
@@ -32,7 +32,7 @@ var mainClosure = function() { | |||
doc = document.createElement('DIV'); | |||
doc.setAttribute('contentEditable', 'true'); | |||
doc.id = 'tempEditDiv'; | |||
doc.setAttribute('style','border: none; padding: 10px 10px 55px; font-size: 20px; outline: none; min-height: calc(100% - 20px); word-wrap: break-word;'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 55px bottom padding is needed for the mobile version. It allows the user to scroll the text above the hide-keyboard button. Actually, it would also be good to use margin-top: 55px;
for the desktop version to solve the same problem.
Now that I think about it, it is probably best to use margin
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yes. Of course, this is easily fixed by adding the padding-bottom if(mobile)
. The reason I didn't do that was
- The editor then needs to know an awful lot about the UI around it, rather than just being a standalone rectangle. This is especially a problem when adding more editors, e.g. [Investigate] WebODF editor #189, [Editor] Implement better code editor #192, Plugins! #129.
- It's slightly unclear where the document actually ends.
Also, theoretically you have the same problem on the desktop (you can't access to top-right corner in zen-mode).
Also, there's the somewhat related problem that it isn't very pleasant to type near the bottom of the screen all the time. Word processors solve this by dividing documents into pages, allowing you to scroll to the bottom of the current page. Some text editors (Sublime Text) solve this by allowing you to scroll one screen height past the end of the file. Line numbers indicate where the document stops.
Maybe it's an idea to increase the padding-bottom by more than 55px, and then add a visual indicator of the borders of the document? (E.g. a real border, or make the padding a different color, light grey or so.) OTOH, that'd make it less WYSIWYG.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re margin
, then you don't use the space anymore for viewing the document, isn't it? It does (mostly) solve the problems I mentioned.
Edit: if you can figure out how to keep the keyboard up, you could even permanently show the toolbar on mobile just as well as using margin
.
When this app was originally coded, mobile was the only target. As a result, it takes a lot of hacks to run Firetext on desktop. I have created an issue for us to discuss desktop strategies here: #203.
What do you think about moving the formatting toolbar into the editor? This would reduce a lot of the cross-frame communication, and allow for different features for different file types. I have created a new issue for editor strategies: #204.
That's an interesting idea. Let's file an issue about it! Do you mind putting this pull request on hold until we can figure out exactly what we need to do? |
Thanks for splitting things up.
Done: #205
No, not really, but IMO #231 is a critical issue and all the others less so. So if things end up taking a long time I think we should merge the two lines of code you proposed in #195. |
To minimize mouse movement.
Could you take another look at this? |
Sure! |
The padding-top is for zen mode, where the toolbar (button) is at the top.
That was intentional, not that important though. I've changed it. |
Only con versus having it in the top-left corner is that, as the number of buttons in there grows, you have to search longer for a button. Doesn't matter now though. |
@twiss: since zen mode is about simplicity, maybe we should only show a select few buttons? |
True. Go ahead, then! |
Good work, @twiss! |
Fixes #195