-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: Allow multiple same properties in single thing #1249
ui: Allow multiple same properties in single thing #1249
Conversation
9a482e4
to
78ebf0e
Compare
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>
|
78ebf0e
to
647ab4f
Compare
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 Report
@@ 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.
|
647ab4f
to
d9f6918
Compare
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>
d9f6918
to
cb9fa76
Compare
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>
cb9fa76
to
dd37b60
Compare
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>
dd37b60
to
3b7dea8
Compare
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>
3b7dea8
to
68f5c51
Compare
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>
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>
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>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
<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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
super(template); | ||
|
||
this._button = this.shadowRoot.querySelector('#button'); | ||
this._name = this.shadowRoot.querySelector('#name'); | ||
this._button = this.shadowRoot.querySelector(`#button${this.idSuffix}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. this becomes this.shadowRoot.querySelector('.webthing-action-button')
.
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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes the next patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
68f5c51
to
6fab303
Compare
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>
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>
6fab303
to
2bb23a5
Compare
Feedback welcome, I plan to add changes for sliders soon |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok to me. @hobinjk do you have any other objections?
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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>
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