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

You can not use :after nor :before if you use tooltips #150

Closed
pjasiun opened this issue Dec 23, 2016 · 16 comments
Closed

You can not use :after nor :before if you use tooltips #150

pjasiun opened this issue Dec 23, 2016 · 16 comments

Comments

@pjasiun
Copy link

pjasiun commented Dec 23, 2016

You can not use :after nor :before if you use tooltips. It means, for instance, that you can not use triangle to indicate that the button is a dropdown if you use tooltip: https://github.com/ckeditor/ckeditor5-theme-lark/blob/78a54836975b699a5862388ff05c0eebbb7cfaa8/theme/components/dropdown.scss#L4-L23 what worked fine before. All other usages of :before or :after will be broken if you use a tooltip. It looks like a big limitation for developers and I think we should consider other (JS) solution for tooltips.

@oskarwrobel
Copy link
Contributor

Yes, and we all agreed with that. We decided to try with simple CSS only tootlips. But now I also think that this is too big limitation.

@pjasiun
Copy link
Author

pjasiun commented Dec 23, 2016

In fact, the problem is not the fact that you can not add :before or :after but the fact that there is no nice way to wrap ButtonView. You can create a View which has a ButtonView as a child, but then it's no longer a ButtonView. You can not use methods nor events of it. You need to use a .button property, what is not cool.

@oskarwrobel
Copy link
Contributor

Some problem is that every button has a tooltip even when the tooltip is not necessary and at default after and before are not available to use.

@Reinmar
Copy link
Member

Reinmar commented Dec 23, 2016

You can create a View which has a ButtonView as a child, but then it's no longer a ButtonView

Can't you wrap it with any class and just provide the same interface? Is there really a problem?

@pjasiun
Copy link
Author

pjasiun commented Dec 23, 2016

It's not that simple to provide the same interface (https://github.com/ckeditor/ckeditor5-ui-default/blob/master/src/button/buttonview.js#L34-L96). You will need to update it every time ButtonView updates.

But what do you think about wrapping the template in such cases:

		this.template = new Template( {
			tag: 'div',
			attributes: {
				class: 'can-have-before-and-after'
			},
			children: [ this.template ]
		} );

I just checked it and it works, with small fixes in the Template class.

@oskarwrobel
Copy link
Contributor

I like it :)

@oskarwrobel
Copy link
Contributor

But this will be a second way to extending a template.

@Reinmar
Copy link
Member

Reinmar commented Dec 23, 2016

But what do you think about wrapping the template in such cases:

I guess it makes a perfect sense.

@oleq
Copy link
Member

oleq commented Jan 3, 2017

@Reinmar
Copy link
Member

Reinmar commented Jan 3, 2017

Hm... this issue is not covered by ckeditor/ckeditor5-ui#119. It will not be covered until we won't rewrite this using JS.

@Reinmar Reinmar reopened this Jan 3, 2017
@Reinmar Reinmar removed this from the iteration 6 milestone Jan 3, 2017
@oleq
Copy link
Member

oleq commented Jan 4, 2017

The question is: is this still a real issue? I fixed

You can not use :after nor :before if you use tooltips. It means, for instance, that you can not use triangle to indicate that the button is a dropdown if you use tooltip:

and it works just fine. And ckeditor/ckeditor5-ui#118 allowed easier manipulation in the template tree, which should solve most of the cases.

@Reinmar
Copy link
Member

Reinmar commented Jan 4, 2017

I think it is. It's not a critical issue, but it's annoying. You can't easily have a dropdown button with a tooltip and triangle. Wrapping it with another element breaks the icon (which, according to what @pjasiun said lands in the wrong place now)...

In fact, I don't like the idea of changing the template of an existing component any more (I must've been sleepy previously) :D. What I initially proposed (#150 (comment)) is far cleaner and safer, although requires maintaining the new interface, so it's less convenient at first.

But there are more solutions or improvements which we can consider.

  • :before and :after should not be taken by tooltips.
  • The icon should be part of the template which would ensure that it lands in the right place even if the template is modified. This means that we'd need conditional rendering of template subtrees.
  • It should be possible to easily override the whole template of a button (in a way that it'd still work), which means that perhaps some functions would need to be exposed.

Finally, I'll repeat this once more – we've got a perfect example here of how subclassing can go wrong. Wrapping the template of an existing element creates a new type of a component because template is its interface too. We can see now that it broke the icon placement (so the internals of this component), but I can easily imagine that it can also break its integration with e.g. dropdown if we were a bit careless and made the dropdown component try adding class to the button (which I think we did in the past).

So:

  • The less often we have to modify any template or other part of the interface the better (hence the need to unblock :before and :after).
  • The smaller the interface the better. We can hide the template (make it protected), but TBH it makes little sense if element stays public. At the same time, being able to actually add a class to an existing button is pretty useful (and doesn't change its interface). So, what we need to avoid is implementing dependencies between components that they require too much knowledge about the element of other components. If anything, they should use an explicit, public API of these components to interact with them. And when theming these relations, we just can't expect that any relations exist :D. In other words – loose coupling FTW.

@pjasiun
Copy link
Author

pjasiun commented Jan 4, 2017

Agree with @Reinmar (and @oskarwrobel who did not like it from the beginning) that wrapping template is not a good practice. The template change is fine, because I still think that using a template as a another template child is fine, but we should not use it for wrapping the views template in subclasses.

@oleq
Copy link
Member

oleq commented Jan 4, 2017

So, in other words, this issue is about converting tooltips into independent JS views at some point in the future?

@pjasiun
Copy link
Author

pjasiun commented Jan 4, 2017

Most probably, if there is no better way to avoid using :after and :before.

@Reinmar
Copy link
Member

Reinmar commented Feb 2, 2017

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

4 participants