ui: Allow multiple same properties in single thing #1249

rzr · 2018-08-02T13:46:54Z

Previously item ids were duplicated,
the observed consequence is that any click on widgets
is seen as click on 1st declared element.

For example, a thing with 2 switches is displayed correctly,
and click events are also triggered but will be "redirected"
to 1st switch only, this is not desired.

Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65
Bug: #1148
Fixes #1148
Origin: https://github.com/tizenteam/gateway
Signed-off-by: Philippe Coval p.coval@samsung.com

Previously item ids were duplicated, the observed consequence is that any click on widgets is seen as click on 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. This change fix it for boolean related widget, and once solution merged, it can be easly replicated to others WoT types (level, text etc). Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

mrstegeman · 2018-08-02T15:12:32Z

@rzr

> eslint .

/home/travis/build/mozilla-iot/gateway/static/js/components/base-component.js
  15:11  error  'idSuffix' is assigned a value but never used. Allowed unused vars must match /^_/  no-unused-vars

/home/travis/build/mozilla-iot/gateway/static/js/components/property/boolean.js
  17:13  error  Infix operators must be spaced            space-infix-ops
  18:5   error  Unexpected var, use let or const instead  no-var

/home/travis/build/mozilla-iot/gateway/static/js/components/property/switch.js
  17:13  error  Infix operators must be spaced            space-infix-ops
  18:5   error  Unexpected var, use let or const instead  no-var

✖ 5 problems (5 errors, 0 warnings)
  4 errors, 0 warnings potentially fixable with the `--fix` option.

Previously item ids were duplicated, the observed consequence is that any click on widgets is seen as click on 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. This change fix it for boolean related widget, and once solution merged, it can be easly replicated to others WoT types (level, text etc). Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

codecov-io · 2018-08-02T15:15:25Z

Codecov Report

Merging #1249 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1249   +/-   ##
=======================================
  Coverage   75.33%   75.33%           
=======================================
  Files         123      123           
  Lines        5916     5916           
  Branches      824      824           
=======================================
  Hits         4457     4457           
  Misses       1287     1287           
  Partials      172      172

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8a383f...68f5c51. Read the comment docs.

Previously item ids were duplicated, the observed consequence is that any click on widgets is seen as click on 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. This change fix it for boolean related widget, and once solution merged, it can be easly replicated to others WoT types (level, text etc). Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

Previously item ids were duplicated, the observed consequence is that any click on widgets is seen as click on 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Slider not fully updated, to keep passing tests, more refactoring to come in that part (grep '#slide') Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Slider not fully updated, to keep passing tests, more refactoring to come in that part (grep '#slider') Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

To avoid clash LevelProperty's id are renamed to slide-level-${count} It was tested on ARTIK05x thing (2 switches, 2 booleans, 2 levels) Change-Id: If530737168af14eb8f6f1e9e230e04a432490045 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Slider not fully updated, to keep passing tests, more refactoring to come in that part (grep '#slider') Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

To avoid clash LevelProperty's id are renamed to slide-level-${count} It was tested on ARTIK05x thing (2 switches, 2 booleans, 2 levels) Change-Id: If530737168af14eb8f6f1e9e230e04a432490045 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

rzr · 2018-08-10T20:31:49Z

If it helps, I can share an example to test using webthing-node (currently using webthing-iotjs)

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. It was tested for boolean, number related components. Slider not fully updated, to keep passing tests, more refactoring to come in that part (grep '#slider') Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

hobinjk

This is a really comprehensive set of changes! I think it might be better to switch the querySelectors to use classes which are naturally less conflicting. I believe the reason for the issue with shadowRoot.querySelector is that if an id conflicts at the base document level there is nothing the webcomponents polyfill can do to constrain the querySelector within the shadowRoot. I'm hoping the behavior will be better with classes and not require switching over so much code to use this idSuffix construction (and introducing weird bugs if one fails to use idSuffix)

hobinjk · 2018-08-22T15:23:17Z

static/js/components/property/action.js

-  <div id="container" class="webthing-action-container">
-    <div id="contents" class="webthing-action-contents">
-      <button id="button" type="button" class="webthing-action-button"></button>
+  <div id="container-${BaseComponent.count}" class="webthing-action-container">


Since all of these elements are retrieved using this.shadowRoot.querySelector I believe a better way of solving the ID conflicts is to use querySelector with the class instead of the id and drop the id altogether.

hobinjk · 2018-08-22T15:23:51Z

static/js/components/property/action.js

    super(template);

-    this._button = this.shadowRoot.querySelector('#button');
-    this._name = this.shadowRoot.querySelector('#name');
+    this._button = this.shadowRoot.querySelector(`#button${this.idSuffix}`);


e.g. this becomes this.shadowRoot.querySelector('.webthing-action-button').

hobinjk · 2018-08-22T15:31:22Z

static/js/components/property/switch.js

          class="webthing-switch-property-switch">
-        <label id="slider" for="switch" class="webthing-switch-property-slider">
+        <label id="slider-${BaseComponent.count}" for="switch-${BaseComponent.count}" class="webthing-switch-property-slider">


Here the input might need to be nested within the label to allow the dropping of the for property which would otherwise be subject to an id conflict.

yes the next patch

Got your point but I fear we still need IDs : see comments at bottom of : https://stackoverflow.com/questions/41811551/custom-styling-for-checkbox-with-input-within-label

I have updated the patch to make it smaller and easier to review, once OK I will generalize this to other widgets.

Previously UI item's ids were duplicated, the observed consequence is that any click on widgets is catched by 1st declared element. For example, a thing with 2 switches is displayed correctly, and click events are also triggered but event will be "redirected" to 1st switch only, this is not desired. The change is only fixing boolean widgets, others to come later. Note: It was suggested to use nested inputs, but unfortunately CSS can't traverse (to input) and get back to parent (label), so "for" facility and id must be used. Change-Id: I27ef75a95889c37224aa7da6b0fbf8e2ad57ba65 Bug: WebThingsIO#1148 Forwarded: WebThingsIO#1249 Origin: https://github.com/tizenteam/gateway Signed-off-by: Philippe Coval <p.coval@samsung.com>

rzr · 2018-09-17T08:29:04Z

Feedback welcome, I plan to add changes for sliders soon

codecov-io · 2018-09-18T22:07:22Z

Codecov Report

Merging #1249 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #1249     +/-   ##
========================================
+ Coverage    75.1%   75.2%   +0.1%     
========================================
  Files         123     123             
  Lines        5989    5989             
  Branches      842     842             
========================================
+ Hits         4498    4504      +6     
+ Misses       1317    1312      -5     
+ Partials      174     173      -1

Impacted Files	Coverage Δ
src/models/things.js	`98.71% <0%> (+1.28%)`	⬆️
src/models/thing.js	`68.62% <0%> (+1.3%)`	⬆️
src/db.js	`82.08% <0%> (+1.73%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a9fee...acf9eb8. Read the comment docs.

mrstegeman

This seems ok to me. @hobinjk do you have any other objections?

codecov-io · 2018-09-18T22:21:56Z

Codecov Report

Merging #1249 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #1249      +/-   ##
=========================================
- Coverage    75.1%     75%   -0.11%     
=========================================
  Files         123     125       +2     
  Lines        5989    6076      +87     
  Branches      842     848       +6     
=========================================
+ Hits         4498    4557      +59     
- Misses       1317    1344      +27     
- Partials      174     175       +1

Impacted Files	Coverage Δ
src/controllers/logs_controller.js	`82.97% <0%> (-13.45%)`	⬇️
src/db.js	`75.53% <0%> (-4.82%)`	⬇️
src/router.js	`96.42% <0%> (-1.69%)`	⬇️
src/plugin/plugin.js	`74.66% <0%> (-0.67%)`	⬇️
src/plugin/addon-manager-proxy.js	`80.91% <0%> (-0.63%)`	⬇️
src/rules-engine/effects/index.js	`100% <0%> (ø)`	⬆️
src/constants.js	`100% <0%> (ø)`	⬆️
src/rules-engine/effects/NotificationEffect.js	`36.36% <0%> (ø)`
src/controllers/push_controller.js	`58.62% <0%> (ø)`
src/plugin/plugin-server.js	`86.66% <0%> (+0.95%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89a9fee...acf9eb8. Read the comment docs.

hobinjk · 2018-09-18T22:22:55Z

I'm still not a fan of introducing BaseComponent.count but that appears to be necessary for this to work so LGTM.

In case of multiple property of same types, UI events were forwarded to 1st instance of widget. This change is a follow up of the switch change: Bug: WebThingsIO#1148 Relate-to: WebThingsIO#1249 Change-Id: I2018092168af14eb8f6f1e9e230e04a432490045 Signed-off-by: Philippe Coval <p.coval@samsung.com>

In case of multiple property of same types, UI events were forwarded to 1st instance of widget. This change is a follow up of the switch change: Origin: WebThingsIO#1370 Bug: WebThingsIO#1148 Relate-to: WebThingsIO#1249 Change-Id: I2018092168af14eb8f6f1e9e230e04a432490045 Signed-off-by: Philippe Coval <p.coval@samsung.com>

* ui: Select by class instead of (multiple) id In case of multiple property of same types, UI events were forwarded to 1st instance of widget. This change is a follow up of the switch change: Origin: #1370 Bug: #1148 Relate-to: #1249 Change-Id: I2018092168af14eb8f6f1e9e230e04a432490045 Signed-off-by: Philippe Coval <p.coval@samsung.com> * ui: Prevent duplicated id This change has no real effect, only for making code more uniform, Note that slider id has been renamed to slider-level to remove ambiguity with slider, and counter suffix adjusted. Forwarded: #1370 Change-Id: I64fe6a78a8ae9d300bab58561a7b0eec7bfb09db Signed-off-by: Philippe Coval <p.coval@samsung.com>

rzr force-pushed the sandbox/rzr/devel/review/master branch from 9a482e4 to 78ebf0e Compare August 2, 2018 13:49

mrstegeman added the review label Aug 2, 2018

rzr force-pushed the sandbox/rzr/devel/review/master branch from 78ebf0e to 647ab4f Compare August 2, 2018 15:15

rzr force-pushed the sandbox/rzr/devel/review/master branch from 647ab4f to d9f6918 Compare August 2, 2018 15:22

rzr force-pushed the sandbox/rzr/devel/review/master branch from d9f6918 to cb9fa76 Compare August 3, 2018 00:01

rzr force-pushed the sandbox/rzr/devel/review/master branch from cb9fa76 to dd37b60 Compare August 3, 2018 08:40

rzr force-pushed the sandbox/rzr/devel/review/master branch from dd37b60 to 3b7dea8 Compare August 3, 2018 17:15

rzr force-pushed the sandbox/rzr/devel/review/master branch from 3b7dea8 to 68f5c51 Compare August 3, 2018 18:13

rzr mentioned this pull request Aug 21, 2018

MultiLevelSensor gets confused with multiple LevelProperties with different units #1148

Closed

hobinjk suggested changes Aug 22, 2018

View reviewed changes

rzr force-pushed the sandbox/rzr/devel/review/master branch from 68f5c51 to 6fab303 Compare September 11, 2018 15:30

rzr force-pushed the sandbox/rzr/devel/review/master branch from 6fab303 to 2bb23a5 Compare September 11, 2018 15:53

Update switch.js

acf9eb8

ghost assigned mrstegeman Sep 18, 2018

mrstegeman reviewed Sep 18, 2018

View reviewed changes

WebThingsIO deleted a comment from codecov-io Sep 18, 2018

hobinjk approved these changes Sep 18, 2018

View reviewed changes

mrstegeman merged commit 6f909b2 into WebThingsIO:master Sep 19, 2018

ghost removed the review label Sep 19, 2018

rzr mentioned this pull request Sep 24, 2018

ui: Select by class instead of (multiple) id #1370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Allow multiple same properties in single thing #1249

ui: Allow multiple same properties in single thing #1249

rzr commented Aug 2, 2018 •

edited by mrstegeman

Loading

mrstegeman commented Aug 2, 2018

codecov-io commented Aug 2, 2018 •

edited

Loading

rzr commented Aug 10, 2018

hobinjk left a comment

hobinjk Aug 22, 2018

hobinjk Aug 22, 2018

hobinjk Aug 22, 2018

rzr Aug 22, 2018

rzr Sep 11, 2018

rzr commented Sep 17, 2018

codecov-io commented Sep 18, 2018

mrstegeman left a comment

codecov-io commented Sep 18, 2018

hobinjk commented Sep 18, 2018

ui: Allow multiple same properties in single thing #1249

ui: Allow multiple same properties in single thing #1249

Conversation

rzr commented Aug 2, 2018 • edited by mrstegeman Loading

mrstegeman commented Aug 2, 2018

codecov-io commented Aug 2, 2018 • edited Loading

Codecov Report

rzr commented Aug 10, 2018

hobinjk left a comment

Choose a reason for hiding this comment

hobinjk Aug 22, 2018

Choose a reason for hiding this comment

hobinjk Aug 22, 2018

Choose a reason for hiding this comment

hobinjk Aug 22, 2018

Choose a reason for hiding this comment

rzr Aug 22, 2018

Choose a reason for hiding this comment

rzr Sep 11, 2018

Choose a reason for hiding this comment

rzr commented Sep 17, 2018

codecov-io commented Sep 18, 2018

Codecov Report

mrstegeman left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 18, 2018

Codecov Report

hobinjk commented Sep 18, 2018

rzr commented Aug 2, 2018 •

edited by mrstegeman

Loading

codecov-io commented Aug 2, 2018 •

edited

Loading