T/857 View placeholder #872

szymonkups · 2017-03-16T14:32:53Z

Suggested merge commit message (convention)

Feature: Added placeholder utility that can be applied to view elements. Closes ckeditor/ckeditor5#4008.

Additional information

Requires #864.

…FocusObserver.

…arguments, added checking function.

pjasiun · 2017-03-17T11:49:15Z

src/view/placeholder.js

+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+
+const CSS_CLASS = 'ck-placeholder';
+const DATA_ATTRIBUTE = 'data-placeholder';


I remember I did something like this in my first PR to the CKEditor 4. And I remember that Fred gave me R- then telling it an unnecessary complication. Now, I see that we use it more and more. And to some point I like it.

But on the other hand, it's 2017 and all of us have IDEs which will easly find for us all data-placeholder and replace with anything we want. Also, I think that if one day we will want to replace it with something else, it probably will be a configurable variable so it will have to be changed anyway. At the same time, the fact that I have data-placeholder in the Chrome dev tools, and DATA_ATTRIBUTE in the code, and need to check and remember that it equals does not help to debug. It might not be hard with one variable, but we get more and more of these. I know, that it's a popular good practice, but, my question is: what do we get?

And I even think I know the answer. These kind of constants is useful to avoid magic strings and magic number. For instance:

const BLOCKS_SPACING = 15; // ... createBlock( BLOCK_SPACING );

is far more readable them:

createBlock( 15 );

But in this case, I'm not sure if these 2 are magic strings. I don't see anything hard to understand in:

element.setAttribute( 'data-placeholder', placeholderText );

I even like it more that the current version, because I immediately know what I need to look for in the browser's inspector in case of debugging.

So my proposal is to use this kind of constants only when the value or/and the context of its usage might be unclear and adding named const improve readability.

I don't see many of this in our code. I've recently used this approach when I was working on CSS classes in image and widgets, but same as you - I have a feeling that we could get rid of this.

pjasiun · 2017-03-17T12:11:20Z

src/view/placeholder.js

+extend( listener, EmitterMixin );
+
+// Each document stores information about its placeholder elements and check functions.
+const documentPlaceholders = new Map();


Why not a WeakMap?

~~You can't iterate over WeakMap items.~~
Now I can see what you mean - I can go without iterating at all :D Good point.

In fact what you need is to add a placeholders property to the document, but in a nice way, without modifying document object. WeakMap do exactly this.

pjasiun · 2017-03-17T12:14:31Z

src/view/placeholder.js

+
+	for ( let placeholders of documentPlaceholders.values() ) {
+		placeholders.delete( element );
+	}


If you would change documentPlaceholders to the WeakMap you will have to change the loop to: documentPlaceholders( element.document ).delete( element );. But I think it worth to change it anyway.

pjasiun · 2017-03-17T12:33:17Z

And one more thought: the view/placeholder.js file is, in fact, a singleton class. With public and private methods, properties. Don't get be wrong, I like how it is implemented, but I might spend too much time with OOP if I see it this way ;)

Reinmar · 2017-03-17T12:48:37Z

And one more thought: the view/placeholder.js file is, in fact, a singleton class.

It's a module (being a file is just a way to express a module). And indeed – a module can be like a singleton class. The exported stuff is the public API, the rest is private. It's exactly the same what we used to do with:

const api = ( function() {
  var priv = [];

  return { 
    add: function( priv ) {
      priv.push( item );
    }
  };
} )();

But a module is nicer :)

pjasiun · 2017-03-21T12:52:34Z

src/view/observer/focusobserver.js

+import global from '@ckeditor/ckeditor5-utils/src/dom/global';
+
+const setTimeout = global.window.setTimeout;
+const clearTimeout = global.window.clearTimeout;


Can we remove this?

pjasiun · 2017-03-21T12:52:59Z

tests/view/observer/focusobserver.js

+import global from '@ckeditor/ckeditor5-utils/src/dom/global';
+
+const setTimeout = global.window.setTimeout;
+const domDocument = global.document;


pjasiun · 2017-03-21T12:53:12Z

tests/view/observer/selectionobserver.js

+const setTimeout = global.window.setTimeout;
+const setInterval = global.window.setInterval;
+const clearInterval = global.window.clearInterval;
+const domDocument = global.document;


And this? ;)

pjasiun · 2017-03-21T13:00:54Z

src/controller/editingcontroller.js

@@ -23,6 +23,7 @@ import {
 } from '../conversion/model-selection-to-view-converters';

 import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
+import '../../theme/theme.scss';


I know that we used to have global theme.scss per repository, but since we use Webpack, we could load styles more locally. This is a perfect example of it. We could have placeholder.scss and import it from placeholder.js. Thanks to these styles will not be added to the build as long as someone will not include the js file (there might be no usage of the placeholder.js) and, what is more important, it's very simple to find styles used in that JS fine. You just go to imports like you would do with a class or function.

//cc @oleq @Reinmar

If it really worked, we could get rid of nightmares like https://github.com/ckeditor/ckeditor5-link/blob/master/theme/theme.scss#L4. Waiting for confirmation from Reinmar, then I'm gonna create an umbrella issue in ckeditor5 because it applies to the entire codebase.

Yep, it works great. I use it a lot in another project.

I second that then.

I was thinking that this might be connected with a way how at the end we will build editor with proper theme. I am not sure if this is defined yet but let's wait for @Reinmar response.

Well, there are important things to remember here.

Importing an SCSS file many times may lead to code duplication and bugs. Even if placeholder.scss will be imported only once, it may be importing other SCSS files which are imported somewhere else already, making those rules appear twice. This is super sad and dangerous. I hit this hard when trying to start importing SCSS files in their proper JS components. It blew up.

The solution here is a refactoring of our SCSS. The code needs to be split into two layers – helpers and components. Helpers must not contain any rules – they should only contain mixins, functions, etc. This means that you will be able to import a helper multiple times without duplicating the code. The components will use helpers to define final CSS rules. However, a single component must not be imported more than once.

The second thing is that we must support multiple themes. No one so far made any research about that and the only related ticket so far is https://github.com/ckeditor/ckeditor5-ui/issues/144. But it clarifies that plugins (and engine) must not depend on a particular theme. This is a long topic and it doesn't make sense to spend time on it now, but how it applies to this particular case? It doesn't. The import can be anywhere. But the SCSS file should, at least for now, have no dependency on the theme-lark package.

That's a good point, actually. If we import things directly, switching to another theme will require changes in each single js file that imports SCSS vs. changing theme.scss file only. I wonder whether this could be possible.

… module.

pjasiun · 2017-03-22T19:07:43Z

The change is fine for me. You can merge it as soon as you rebase the branch.

pjasiun · 2017-03-22T19:23:56Z

Nevermind.

szymonkups added 8 commits March 7, 2017 12:45

Added placeholder utility initial implementation and manual test.

0e69c07

Merge branch 't/795' into t/857

1c400c6

Removed focus listener from placeholder - now rendering is called by …

fb082b4

…FocusObserver.

Merge branch 't/795' into t/857

e495c84

Updated attachPlaceholder function API: removed document from needed …

5800d43

…arguments, added checking function.

Merge branch 't/795' into t/857

1553557

Refactored view placeholder code.

1d80bf1

Added tests to views placeholder utility.

7eef995

szymonkups requested a review from Reinmar March 16, 2017 14:32

szymonkups added 2 commits March 16, 2017 16:20

Removed dependency from theme-lark package.

1a4ab25

Merge branch 't/795' into t/857

c6de70d

pjasiun reviewed Mar 17, 2017

View reviewed changes

Fixed code issues in view placeholder.

db3a139

pjasiun reviewed Mar 21, 2017

View reviewed changes

pjasiun added the status:review- label Mar 21, 2017

szymonkups added 2 commits March 22, 2017 10:31

Merge branch 't/795' into t/857

4347ddd

Created placeholder.scss file and importing it directly in placehlder…

12b7cee

… module.

szymonkups removed the status:review- label Mar 22, 2017

pjasiun added the status:review+ label Mar 22, 2017

Merge branch 'master' into t/857

010fc06

pjasiun merged commit 79b42da into master Mar 22, 2017

pjasiun deleted the t/857 branch March 22, 2017 19:23

pjasiun removed the status:review+ label Mar 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/857 View placeholder #872

T/857 View placeholder #872

szymonkups commented Mar 16, 2017 •

edited by pomek

Loading

pjasiun Mar 17, 2017

pjasiun Mar 17, 2017

szymonkups Mar 17, 2017

pjasiun Mar 17, 2017

szymonkups Mar 17, 2017 •

edited

Loading

pjasiun Mar 17, 2017

pjasiun Mar 17, 2017

pjasiun commented Mar 17, 2017 •

edited

Loading

Reinmar commented Mar 17, 2017 •

edited

Loading

pjasiun Mar 21, 2017

pjasiun Mar 21, 2017

pjasiun Mar 21, 2017

pjasiun Mar 21, 2017 •

edited

Loading

oleq Mar 21, 2017

pjasiun Mar 21, 2017

oleq Mar 21, 2017

szymonkups Mar 22, 2017

Reinmar Mar 22, 2017

oleq Mar 22, 2017

pjasiun commented Mar 22, 2017

pjasiun commented Mar 22, 2017 •

edited

Loading

T/857 View placeholder #872

T/857 View placeholder #872

Conversation

szymonkups commented Mar 16, 2017 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymonkups Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun commented Mar 17, 2017 • edited Loading

Reinmar commented Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun commented Mar 22, 2017

pjasiun commented Mar 22, 2017 • edited Loading

szymonkups commented Mar 16, 2017 •

edited by pomek

Loading

szymonkups Mar 17, 2017 •

edited

Loading

pjasiun commented Mar 17, 2017 •

edited

Loading

Reinmar commented Mar 17, 2017 •

edited

Loading

pjasiun Mar 21, 2017 •

edited

Loading

pjasiun commented Mar 22, 2017 •

edited

Loading