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

Implement a creator with sticky toolbar #135

Closed
oleq opened this issue Apr 7, 2016 · 17 comments
Closed

Implement a creator with sticky toolbar #135

oleq opened this issue Apr 7, 2016 · 17 comments
Assignees
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@oleq
Copy link
Member

oleq commented Apr 7, 2016

The idea is to implement a creator which is a hybrid between CKEditor 4 "inline" and "classic" implementations:

  • Inline content (<div contenteditable="true"...).
    • No content scrolling.
  • "Classic" toolbar.
    • Always visible (regardless of focus).
    • 100% content width.
  • Toolbar sticks to the edge of the viewport when content goes off screen, i.e. when a page is scrolled.

image

@oleq oleq self-assigned this Apr 7, 2016
@oleq oleq added the type:task This issue reports a chore (non-production change) and other types of "todos". label Apr 7, 2016
@Reinmar
Copy link
Member

Reinmar commented Apr 7, 2016

Always visible (regardless of focus).

I proposed that, but I'm now a bit unsure whether it should also be visible when the viewport is scrolled, so the toolbar is in sticky mode. Perhaps in this case it may disappear.

@oleq
Copy link
Member Author

oleq commented Apr 7, 2016

So...

  • sticky:
    • yes:
      • visible as long as editable is focused
    • no:
      • always visible

@fredck
Copy link
Contributor

fredck commented Apr 7, 2016

In the classic editor I think it should be always visible regardless of the focus and yes, sticky, up to a specific size, when it then disappears with the content on scroll.

When it comes to inline, we're also taking in consideration the "stick to the bottom when possible" approach, like CKEditor 4, right?

@oleq
Copy link
Member Author

oleq commented Apr 7, 2016

In the classic editor I think it should be always visible regardless of the focus

Then, when editable area of off the screen and user performs non-editing actions, CKEditor would persist and consume viewport space, sticky to the edge of the screen. So I'd rather see the toolbar hidden when off the viewport and editable blurred.

When it comes to inline, we're also taking in consideration the "stick to the bottom when possible" approach, like CKEditor 4, right?

Yes.

@fredck
Copy link
Contributor

fredck commented Apr 7, 2016

Then, when editable area of off the screen and user performs non-editing actions, CKEditor would persist and consume viewport space, sticky to the edge of the screen.

You cut the rest of my comment, which is all about that :D

If the editable goes off screen, the toolbar goes with it. It will bring a more natural feeling to the classic editor. After all, when the editor is totally visible, the toolbar visibility doesn't depend on focus.

Maybe I'm not expressing myself properly. I'll try sketch it out and add a new comment here.

@fredck
Copy link
Contributor

fredck commented Apr 7, 2016

Here is what I wanted to say about the behavior of the classic editor toolbar:

image

@Reinmar
Copy link
Member

Reinmar commented Apr 7, 2016

When it comes to inline, we're also taking in consideration the "stick to the bottom when possible" approach, like CKEditor 4, right?

I don't think we have to do it. For me the behaviour of CSS's position:sticky is enough. Less code, less problems.

@Reinmar Reinmar added this to the 0.1.0 milestone Apr 7, 2016
@fredck
Copy link
Contributor

fredck commented Apr 7, 2016

I don't think we have to do it. For me the behaviour of CSS's position:sticky is enough. Less code, less problems.

Well, I would not count on less real problems... as soon as the editable gets too small at the top of the page or when positioned close to the top of the viewport, we'll then start to have problems.

This is not an issue with the classic editor because the toolbar has a "reserved" space with it. That's not the case with inline.

And as a last comment, I've been always very proud about the behavior in v4... we would be doing a step backwards on something that works great there (and stable, so we don't have to really code everything from scratch).

@Reinmar
Copy link
Member

Reinmar commented Apr 7, 2016

Well, I would not count on less real problems... as soon as the editable gets too small at the top of the page or when positioned close to the top of the viewport, we'll then start to have problems.

I think you should check how position:sticky works :P. It does not matter where the editor is located, because the toolbar has its space above it. Exactly like:

This is not an issue with the classic editor because the toolbar has a "reserved" space with it. That's not the case with inline.

But we're not creating an inline (in CKE4's terms) editor here. We want a boxed UI with an inline editable.

And as a last comment, I've been always very proud about the behavior in v4... we would be doing a step backwards on something that works great there (and stable, so we don't have to really code everything from scratch).

The CKE4's implementation is super complex and has been buggy and problematic as well. The less JS, the more on CSS, the better.

The only way how no positioning below the editor could be a step backwards is that edge case when you scroll the page so low that there's just few pixels of the editable visible at the top of your viewport. That's, IMO, invalid case, because you can always scroll back.

@oleq
Copy link
Member Author

oleq commented Apr 7, 2016

The CKE4's implementation is super complex and has been buggy and problematic as well. The less JS, the more on CSS, the better.

👏

I would start with something simple (we've never used position: sticky before), evaluate pros and cons and then, implement something with JS if necessary.

@fredck
Copy link
Contributor

fredck commented Apr 7, 2016

After a talk with @Reinmar it has been clarified that this issue has nothing to do with "inline editor" (in the v4 point of view), so in that case, it certainly doesn't make sense to go with the "stick-to-the-bottom" thing, as I exemplified myself in my previous comment.

@oleq
Copy link
Member Author

oleq commented May 9, 2016

At the moment the architecture of the UI looks the following way:

image

It's neither perfect nor simple but it's very flexible and, when eventually backed by proper documentation, I believe it will suffice our needs.

@pjasiun
Copy link

pjasiun commented May 9, 2016

Note: #140. I do not think it will be an issue since there should be no problem to make EditableUI extends EditableElement.

@oleq
Copy link
Member Author

oleq commented May 10, 2016

Editable is not a big deal at the moment. We'll see (but I'm OK with it).

@oleq
Copy link
Member Author

oleq commented May 10, 2016

By the way: the model structure and flow in my previous comment is a makeshift decision and may (should?) change. The point is that the editor still waits for features and there isn't much to store and share in those models at the moment. So choices like "use BoxedEditorUI instance as a model for its View" may not be valid in the future because a separate model may be needed for some reason, etc.

@Reinmar
Copy link
Member

Reinmar commented May 10, 2016

Note: #140. I do not think it will be an issue since there should be no problem to make EditableUI extends EditableElement.

EditableUI is a UI controller (so it inherits from ui/Controller), so it cannot extend EditableElement at the same time. What you meant (I assume) was to make the current Editable class inherit from EditableElement. Am I right?

@pjasiun
Copy link

pjasiun commented May 10, 2016

EditableUI is a UI controller, so it cannot extend EditableElement at the same time. What you meant (I assume) was to make the current Editable class inherit from EditableElement. Am I right?

My bad. I misunderstood diagram, thought that InlineEditableUIView inherit from EditableUI which inherit from Editable. Of course there is no reason to make EditableUI extend EditableElement.

We will see if the Editable class will inherit from EditableElement. It may happen that EditableElement will be observable and it will be enough. Anyway, I believe it is discussion for another topic, here I wanted only mention that changes are coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

4 participants