llaske / sugarizer

Sugarizer is a web implementation of the Sugar platform to run on any device or browser
https://sugarizer.org
Apache License 2.0
197 stars 411 forks source link

Mistake in Activity Tutorial #1355

Closed UtkarshSiddhpura closed 1 year ago

UtkarshSiddhpura commented 1 year ago

Inside step8 of activity tutorial

The actual parameter count initialized here for pawnClickEvent https://github.com/llaske/sugarizer/blob/9630e2a2cbab435a65e2239bdac25b13a897a630/docs/tutorial/VueJS/step8.md?plain=1#L183

is stored inside of this.pawnClickEvent.detail.count not in this.pawnClickEvent.count https://github.com/llaske/sugarizer/blob/9630e2a2cbab435a65e2239bdac25b13a897a630/docs/tutorial/VueJS/step8.md?plain=1#L191

It may leads to bugs if someone wants to use previously initialized value. (e.g this.pawnClickEvent.count += 3 // NaN) as count is undefined initially

rockharshitmaurya commented 1 year ago

hey @UtkarshSiddhpura

I think the statement that count is not stored in this.pawnClickEvent.count is not accurate. While it's true that the count parameter is stored in the detail property of the event object, it's also accessible via the count property of the event object itself. therefore, the code should work as expected, and there should not be any bugs caused by accessing this.pawnClickEvent.count after the event is initialized.

llaske commented 1 year ago

The count initialization in initCustomEvent is not important. What's matter is the work done in click event listener methods for items. The count in these methods is set as the place expected by onAddClick method. So, it's okay.

UtkarshSiddhpura commented 1 year ago

While it may not be crucial for the demo activity, providing information on where the initial event parameters are stored is important for the purpose of enabling anyone to easily implement a Sugarizer activity.

Jst for example : chart activity text palette

llaske commented 1 year ago

It's not a good idea to store the current font size in the event. It depends of the context, not of the palette. Have a look on the implementation of increase/decrease font in activities Write, LabyrinthJS or Story, you will see that the current font size always depend of the item so it's always store in the activity.

UtkarshSiddhpura commented 1 year ago

Ok! i get it now