Add sortby state to things-list and extend things-inbox for multi-unignore #1096
Conversation
Signed-off-by: Christian Brosius <Christian.Brosius@gmail.com>
add toggleAll to Multiselect in thing-inbox change Text from 'select' to 'multiselect' Signed-off-by: Christian Brosius <Christian.Brosius@gmail.com>
There was a problem hiding this comment.
Thanks for the PR, I left some comments.
You also need to sign-off your work, see https://www.openhab.org/docs/developer/contributing.html.
bundles/org.openhab.ui/web/src/pages/settings/things/inbox/inbox-list.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/pages/settings/things/inbox/inbox-list.vue
Outdated
Show resolved
Hide resolved
| @@ -23,6 +23,9 @@ | |||
| <f7-link color="orange" v-show="selectedItems.length" v-if="!$theme.md" class="ignore" @click="confirmActionOnSelection('ignore')" icon-ios="f7:eye_slash" icon-aurora="f7:eye_slash"> | |||
| Ignore {{ selectedItems.length }} | |||
| </f7-link> | |||
| <f7-link color="blue" v-show="selectedItems.length" v-if="!$theme.md" class="unignore" @click="confirmActionOnSelection('unignore')" icon-ios="f7:eye" icon-aurora="f7:eye"> | |||
There was a problem hiding this comment.
I'm not sure if there's enough room on a phone screen for 4 links at the bottom - that's why I left the unignore feature out. Unignore could however replace ignore if all checked items are already ignored.
There was a problem hiding this comment.
On my phone (Android10) there is enough space because the Text is not shown.
But this might be depending on the phone or theme.
When starting, the absence of an 'unignore'-button is not cruical, but over time one may add more and more things to the ignore-list, and then it makes much more sense to have 'unignore' in the multiselect-view, too.
There was a problem hiding this comment.
Yep but for instance on a iPhone 12 mini (which would have the iOS theme and a smallish screen), it would look like this:
this part of the code is for themes other than Material Design (i.e. Android) so adding a new function needs to account for those as well.
| @@ -61,6 +65,7 @@ | |||
| By binding | |||
| </f7-button> | |||
| </f7-segmented> | |||
| <f7-checkbox v-if="showCheckboxes" @change="masterToggle" /> | |||
There was a problem hiding this comment.
Not convinced this lone checkbox is the best design for a "select all" feature honestly.
There was a problem hiding this comment.
I agree, but until now I could not find a better, simple way to provide the functionality.
I tried a header-row, but that needs much more space without additional improvements.
Do you have a suggestion for this?
There was a problem hiding this comment.
Not completely sure but I was thinking about "Select All/Unselect All" buttons like those you have on that screen when you add points to the model from a thing:
Though your approach of a "select all" checkbox isn't bad either, maybe it could be made clearer, but in any case we need a common approach. So I would suggest adding these buttons at the bottom of the list when the "select" mode is active.
There was a problem hiding this comment.
What do you think about such a composition?
This way it would be possible to select all items in bigger lists without scrolling to the bottom.
| this.checkAll = !this.checkAll | ||
| for (let e in this.inbox) { | ||
| if (this.checkAll) { | ||
| this.selectedItems.push(this.inbox[e].thingUID) |
There was a problem hiding this comment.
If an item is already in the selectedItems array it will be added again with this code.
There was a problem hiding this comment.
I´m a newbie in vue/javascript programming and contributing. Hopefully this explains a little bit why there are so many issues with my commit :O)
Would it be a solution to empty the selectedItems-Array before setting all items again, or is there a better way to add all items in the list only once?
There was a problem hiding this comment.
I´m a newbie in vue/javascript programming and contributing. Hopefully this explains a little bit why there are so many issues with my commit :O)
No worries, first-time contributions are welcome, we'll walk you through this 😃
There was a problem hiding this comment.
| this.selectedItems.push(this.inbox[e].thingUID) | |
| masterToggle() { | |
| this.checkAll = !this.checkAll | |
| this.selectedItems = [] | |
| if (this.checkAll) { | |
| for (let e in this.inbox) { | |
| this.selectedItems.push(this.inbox[e].thingUID) | |
| } | |
| } | |
| }, | |
bundles/org.openhab.ui/web/src/pages/settings/things/things-list.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/pages/settings/things/things-list.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/pages/settings/things/things-list.vue
Outdated
Show resolved
Hide resolved
bundles/org.openhab.ui/web/src/pages/settings/things/things-list.vue
Outdated
Show resolved
Hide resolved
…ox-list.vue fix typo
|
After writing all the comments I realized, that I already mixed up this PR with another one I wanted to do afterwards. How should I proceed? Is it better to close this one and create two new separate PRs? |
cbrosius
changed the title
And sorry for the delay; on principle, it's indeed always better not to bundle several changes in the same PR as the maintainer (that would be me 😅) might welcome a laser-focused fix for a specific issue but be reluctant if it's accompanied by some other unrelated and potentially unwanted changes. Nevertheless I believe we can make it work. |
|
Any hope for this PR being merged? |
Fixes #668