Make class properties observable #104

cherifGsoul · 2020-04-06T18:32:10Z

This makes class properties observable:

class MyElement extends StacheElement {
		greetings = 'Hello';
                static get view() { return {{ greetings }}}
		static get props() {
		}
}
customElements.define('my-element', MyElement)

In case of:

class MyElement extends StacheElement {
		greetings = 'Hello';
                static get view() { return {{ greetings }}}
		static get props() {
                       return {
				greetings: {type: String, default: 'Bonjour'}
			};
		}
}
customElements.define('my-element', MyElement)

It set the value of props.greetings by the classe property value.

For canjs/can-observable-mixin#79

phillipskevin

Good start @cherifGsoul!

phillipskevin · 2020-04-09T13:31:14Z

src/mixin-props-test.js

@@ -20,3 +20,61 @@ QUnit.test("basics", function(assert) {
 	el.age = "33";
 	assert.equal(el.age, 33, "updated age");
 });
+
+try {


I think all of the tests that require class fields should go in their own file. Then the main test file can do this feature detection and create a variable like supportsClassFields. The wrap the require statement for the tests like

if(supportsClassFields) { require(...); }

phillipskevin · 2020-04-09T13:32:11Z

src/mixin-props-test.js

+		QUnit.test('Class fields should be observable', (assert) => {
+			class DefineElement extends mixinDefine(Object) {
+				greetings = 'Hello'; // jshint ignore:line
+				static get props() {


Are props required?

phillipskevin · 2020-04-09T13:33:54Z

src/mixin-props-test.js

+			const el = new DefineElement();
+
+			if (el.hasOwnProperty('greetings')) {
+				el.initialize();


You should probably check that the default value is correct.

phillipskevin · 2020-04-09T13:34:20Z

src/mixin-props-test.js

+				el.initialize();
+
+				el.on('greetings', () => {
+					assert.ok('The class field is observable');


Can you also check that the value and old value are correct?

phillipskevin · 2020-04-09T13:36:06Z

src/mixin-props-test.js

+		}
+
+		const el = new DefineElement();
+			el.initialize();


You also need to check the value of greetings to make sure the precedence is correct.

phillipskevin · 2020-04-09T13:36:33Z

src/mixin-props-test.js

+				assert.ok('The class field is observable');
+			});
+
+			el.greetings = 'Hola';


After this you should also check that setting the value to a non-string causes an error.

phillipskevin · 2020-04-09T13:37:06Z

src/mixin-props-test.js

+					assert.ok('The class field is observable');
+				});
+
+				el.greetings = 'Hola';


Can you also check that setting the value to a non-string works since expando properties like this are not typed.

src/mixin-props.js

phillipskevin

Added some comments on the docs.

Will review the code separately.

phillipskevin · 2020-05-20T03:25:43Z

docs/can-stache-element.md

@@ -206,6 +206,31 @@ document.body.querySelector("button#remove").addEventListener("click", () => {
 @codepen
 @highlight 14-23,only

+
+#### Observable class fields


Can you move this under the Defining an element's properties heading?

phillipskevin · 2020-05-20T03:26:13Z

docs/can-stache-element.md

+
+#### Observable class fields
+
+`StachElement` class fields are observables like [can-stache-element/static.props static props],


Can you link class fields to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields?

phillipskevin · 2020-05-20T03:26:34Z

docs/can-stache-element.md

+</script>
+```
+@codepen
+@highlight 11,only


The highlighting is off here.

phillipskevin · 2020-05-20T03:27:28Z

docs/can-stache-element.md

+    <button on:click="this.increment()">+1</button>
+  `;
+
+  count: 6


This should be count = 6;.

phillipskevin · 2020-05-21T02:00:57Z

docs/can-stache-element.md

+  count: 6
+}
+customElements.define("count-er", Counter);
+</script>


Can you also add a note in that you must either add the element to the page or call initialize in order for the prop value to be observable?

cherifGsoul · 2020-05-21T21:27:52Z

@phillipskevin I updated the docs!

phillipskevin

Just a couple small comments.

phillipskevin · 2020-06-04T02:57:38Z

docs/can-stache-element.md

+@codepen
+@highlight 11,only
+
+In order to make `count` property observable, either `count-er` element must be added to the page


Can you put this in a blockquote? > and add Note: to the beginning?

phillipskevin · 2020-06-04T02:58:30Z

docs/can-stache-element.md

@@ -135,6 +135,33 @@ customElements.define("count-er", Counter);
 @codepen
 @highlight 9-11,only

+#### Observable class fields
+
+`StachElement` [class fields](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Class_fields) like [can-stache-element/static.props static props],


I think this is missing some words.

class fields are observable like...

Also, I would just leave off the like static props piece. Just

StacheElement class fields are also observable.

...along with the example should be fine for now.

phillipskevin

Found one more issue.

phillipskevin · 2020-06-04T03:22:39Z

test/test.js

+if (supportsClassFields) {
+	//It doesn't work with require
+	//Even when change the above imports to require
+	steal.import('~/src/mixin-props-class-fields-test');


I just tried this out in the canjs/canjs suite and this file can't be loaded since the ~ is the canjs folder. We'll need to figure out how to do this.

phillipskevin

It looks like you added steal-conditional and created the supports-class-fields module, but it is not being used to import the class field test file.

Also, there are still a couple issues with the docs that need to be cleaned up.

cherifGsoul · 2020-06-11T17:19:44Z

It looks like you added steal-conditional and created the supports-class-fields module, but it is not being used to import the class field test file.

Also, there are still a couple issues with the docs that need to be cleaned up.

@phillipskevin I updated the docs
The test file was not in the commit, I added it.

phillipskevin

A couple more small things.

phillipskevin · 2020-06-11T17:27:44Z

docs/can-stache-element.md

@@ -135,6 +135,33 @@ customElements.define("count-er", Counter);
 @codepen
 @highlight 9-11,only

+#### Observable class fields
+
+`StachElement` class fields are also observable.,


can you add the link to class fields docs on MDN?

@phillipskevin Done!

phillipskevin · 2020-06-11T17:28:19Z

test/test.js

@@ -1,3 +1,14 @@
+let supportsClassFields;


This can be removed now, right?

@phillipskevin Done!

phillipskevin

🎉

Make classe properties observable

af40f9b

cherifGsoul requested a review from phillipskevin April 6, 2020 18:32

cherifGsoul added 3 commits April 6, 2020 19:35

Ignore fs-events in detect-cyclic-packages

11ccdc3

remove babel plugin-proposal-class-properties

106291a

Skip test on non-supported browsers

8e69366

phillipskevin requested changes Apr 9, 2020

View reviewed changes

cherifGsoul added 2 commits April 9, 2020 20:26

Put the tests in their own file

fb19a30

Change to import

857378d

cherifGsoul requested a review from phillipskevin April 9, 2020 19:57

cherifGsoul added 4 commits April 10, 2020 20:26

Organie the tests

a045de5

add another test

0cbaec7

Remove useless test

0994e0c

Add documentation for class fields

8dd254e

phillipskevin requested changes May 20, 2020

View reviewed changes

phillipskevin reviewed May 21, 2020

View reviewed changes

Update class fields docs

753a117

cherifGsoul requested a review from phillipskevin May 21, 2020 21:58

Fix link

4c38fc4

phillipskevin requested changes Jun 4, 2020

View reviewed changes

cherifGsoul added 3 commits June 9, 2020 17:33

Update how to require/import test

a21beaa

Fix dynamic importing

be6048b

Use steal-conditional for safari

6042ae4

cherifGsoul requested a review from phillipskevin June 10, 2020 20:02

phillipskevin requested changes Jun 11, 2020

View reviewed changes

cherifGsoul added 2 commits June 11, 2020 18:08

Commit test changes

98759e9

Update docs

975d99d

cherifGsoul requested a review from phillipskevin June 11, 2020 17:18

phillipskevin requested changes Jun 11, 2020

View reviewed changes

Minor changes

6beaabb

cherifGsoul requested a review from phillipskevin June 11, 2020 17:49

phillipskevin approved these changes Jun 11, 2020

View reviewed changes

cherifGsoul merged commit 4595991 into master Jun 11, 2020

cherifGsoul deleted the class-fields-props branch June 11, 2020 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make class properties observable #104

Make class properties observable #104

cherifGsoul commented Apr 6, 2020 •

edited

Loading

phillipskevin left a comment

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin Apr 9, 2020

phillipskevin left a comment •

edited

Loading

phillipskevin May 20, 2020

cherifGsoul May 21, 2020

phillipskevin May 20, 2020

cherifGsoul May 21, 2020

phillipskevin May 20, 2020

cherifGsoul May 21, 2020

phillipskevin May 20, 2020

cherifGsoul May 21, 2020

phillipskevin May 21, 2020

cherifGsoul May 21, 2020

cherifGsoul commented May 21, 2020

phillipskevin left a comment

phillipskevin Jun 4, 2020

phillipskevin Jun 4, 2020

phillipskevin Jun 4, 2020

phillipskevin left a comment

phillipskevin Jun 4, 2020

phillipskevin left a comment

cherifGsoul commented Jun 11, 2020

phillipskevin left a comment

phillipskevin Jun 11, 2020

cherifGsoul Jun 11, 2020

phillipskevin Jun 11, 2020

cherifGsoul Jun 11, 2020

phillipskevin left a comment


		#### Observable class fields

		`StachElement` class fields are observables like [can-stache-element/static.props static props],

Make class properties observable #104

Make class properties observable #104

Conversation

cherifGsoul commented Apr 6, 2020 • edited Loading

phillipskevin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipskevin left a comment • 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

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

cherifGsoul commented May 21, 2020

phillipskevin left a 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

phillipskevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipskevin left a comment

Choose a reason for hiding this comment

cherifGsoul commented Jun 11, 2020

phillipskevin left a 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

phillipskevin left a comment

Choose a reason for hiding this comment

cherifGsoul commented Apr 6, 2020 •

edited

Loading

phillipskevin left a comment •

edited

Loading