Skip to content
This repository has been archived by the owner on Nov 16, 2017. It is now read-only.

Implement contextual toolbar component sample #131

Closed
oleq opened this issue Nov 29, 2016 · 8 comments
Closed

Implement contextual toolbar component sample #131

oleq opened this issue Nov 29, 2016 · 8 comments

Comments

@oleq
Copy link
Member

oleq commented Nov 29, 2016

I.e. to handle cases like this

contextual-toolbar-demo

@oleq
Copy link
Member Author

oleq commented Dec 2, 2016

Awaits #98.

@oleq
Copy link
Member Author

oleq commented Dec 7, 2016

A demo of the toolbar is in http://localhost:8125/tests/ui-default/createcontextualtoolbar/createcontextualtoolbar.html brought by #135.

contextual-toolbar-demo

TBH, given that each integration using such a toolbar has totally different behavioral requirements, e.g.

  1. The Image feature will always attach it to a widget element (N,S,W,E) but never to other selections, hide when a widget is blurred.
  2. Inline styles editing will attach it to the range (N at the beginning of the range, S at the end, like in the GIF) when selection is non–collapsed, hidden otherwise.

etc., I'm not quite sure whether it should become a component because the logic controlling it will be dramatically different in each case. It's isn't so much code after all.

WDYT?

@fredck
Copy link

fredck commented Dec 7, 2016

I have a feeling that the only repeatable behavior will be the one implemented by the Image feature (top+center). Theoretically, all other widgets like features will follow the same behavior, for UI consistency Therefore this case may be a component.

The text selection case instead seems to be not repeatable by different features. So a component for it may not make sense, in fact.

@Reinmar
Copy link
Member

Reinmar commented Dec 7, 2016

Positioning is not a component. It's a trait, not a thing. So we need a toolbar + balloon + various positioning behaviours. IMO, the last thing is just a set of helpers. (that said, I haven't seen any code yet, so ignore me if my comment makes little sense)

@Reinmar
Copy link
Member

Reinmar commented Dec 7, 2016

I've seen the code now and I'm now convinced that this doesn't deserve a component. I'd even leave the code now where it is (in the manual test). I see two solutions for the future:

  • Either make a positioning helper,
  • or convert it to a sample.

The latter may be the best solution. We planned to implement samples in the same form as tests – so each contains a JS file, HTML file and MD file. So each sample will also be a manual and/or automated test for itself. We even implemented such a system, but after we dropped Bender and stopped working on the docs builder that code certainly stopped working :(. However, I see this as inevitable that we'll have testable samples. So, if we don't see this code very useful and definitive (i.e. that it implements the one and only contextual toolbar) there will be a place for it as a sample (cookbook style).

@oleq
Copy link
Member Author

oleq commented Dec 8, 2016

I've seen the code now and I'm now convinced that this doesn't deserve a component.

Even that one for the image?

Either make a positioning helper,

There's positioning helper utility now, which can easily be used. So why bother with another layer of abstraction?

@Reinmar
Copy link
Member

Reinmar commented Dec 8, 2016

I've seen the code now and I'm now convinced that this doesn't deserve a component.

I don't think so. Favour composition over inheritance. It's still a toolbar in a balloon panel, just displayed in a specific way, responding to events in a specific way.

There's positioning helper utility now, which can easily be used. So why bother with another layer of abstraction?

That's not abstraction. That's exactly the opposite – concretisation. The code you mentioned is pretty long – around 50LOC. We can either let people copy&paste it around their code bases or close it within a helper.

@oleq oleq changed the title Implement contextual toolbar component Implement contextual toolbar component sample Dec 8, 2016
@oleq
Copy link
Member Author

oleq commented Dec 16, 2016

Closed in #135.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants