-
-
Notifications
You must be signed in to change notification settings - Fork 243
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
Add new capabilities to oh-knob component using vue-round-slider #1703
Add new capabilities to oh-knob component using vue-round-slider #1703
Conversation
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Job #771: Bundle Size — 16.18MiB (+1.3%).Metrics (4 changes)
Total size by type (2 changes)
|
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.
Overall LGTM, thanks!
The dottedPath
option for the slider-control does also apply to the knob which causes some design problems - I will take care of that.
And I also have a few parameter naming improvements in mind.
<knob-control v-if="!config.sliderType" v-bind="config" :text-color="config.textColor || (($f7.data.themeOptions.dark === 'dark') ? '#ffffff' : undefined)" :value="value" | ||
@input="sendCommandDebounced($event)" @click.native.stop="sendCommandDebounced(value, true)" @touchend.native.stop="sendCommandDebounced(value, true)" /> | ||
|
||
<round-slider v-else :value="value" :min="config.min" :max="config.max" :step="config.stepSize" :radius="config.size/2" |
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.
Using v-bind
here would have been nice, but I see the reason why you did not: several paramaters have different names compared to the prop names. Therefore I am fine with that!
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, exactly, that's why I couldn't use v-bind.
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
The strokeWidth param as string caused Vue warnings logged to the console. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Thanks guys !!!!!! |
@florian-h05 @stefan-hoehn A word of advice when adding new libraries. The RelativeCI bot was added (#1703 (comment)) to track the size of the overall app and the "initial JS/CSS". Here we can see that the Initial JS has increased ( This is illustrated if you open the "report" artifact from the GitHub build results: https://github.com/openhab/openhab-webui/actions/runs/4157806937 The purple block in the top-left corner in the JS entry point and the added weight of vue-roundslider is highlighted). See for example: openhab-webui/bundles/org.openhab.ui/web/src/components/widgets/system/oh-video.vue Lines 29 to 32 in 5bf9845
In this case maybe it's acceptable (or maybe not, in fact) but do keep that in mind. |
Thanks for the advice. Wasn‘t aware of that size increase — I guess it‘s time for a follow-up PR to split off that component. |
No worries, the reasons I'm telling you this is because while the overall size of the UI has increased over the years, I've tried to keep the baseline to a minimum. For instance compare https://app.relative-ci.com/projects/ZNG5hy4VeSJQVQcq1Kvu/jobs/9-85f49c87-0ed2-470c-95e5-a0a4edad8145: to a job from 6 days ago https://app.relative-ci.com/projects/ZNG5hy4VeSJQVQcq1Kvu/jobs/761-CfxT1mQIUU2wyVZ1P1hW: The overall size has indeed increased from ~11MB to ~16MB as more features were added, but the "initial JS" has been kept to a minimum: 1.73MB vs. 1.67MB! So the more we can keep (or carve) out of it (with common sense, some things belong in it), the better 🙂! |
Follow-up PR for openhab#1703. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@ghys PR #1718 implements lazy loading for the about 130 kB large |
thanks, @florian-h05 , you are too fast for me ;-) I wanted to do it today, so thanks for taking care! |
Follow-up PR for openhab#1703. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Follow-up PR for openhab#1703. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Follow-up PR for openhab#1703. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
… dependencies (#1718) Follow-up PR for #1703. This: - lazy loads `vue-knob-control` - refactors `vue-round-slider` to only use `vue-round-slider` and remove `vue-knob-control` A compatiblity layers ensures that existing `oh-knob` configurations still work and default values of properties have been adjusted so that `vue-round-slider` looks like the removed `vue-knob-control`. Paremeter description and docs were updated accordingly to `vue-round-slider`. -- Signed-off-by: Florian Hotze <florianh_dev@icloud.com> Also-by: Dan Cunningham <dan@digitaldan.com>
fixes #1640
Enhanced the oh-know via the rounded-slider component and add several new capabilities.
See details at #1640 and examples there.