Skip to content
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

Rework oh-knob and oh-slider, fixes #1003 #1012

Merged
merged 1 commit into from
May 16, 2021
Merged

Conversation

hubsif
Copy link
Contributor

@hubsif hubsif commented Apr 20, 2021

I reworked oh-knob in order to fix #1003, which resulted following:

  • commands are now sent in reliable intervals, i.e. a minimum time must have past before another command gets sent (this has not fully been the case before)
  • if the change, e.g. a click, happens faster than that interval, the command gets sent immediately
  • in order for above to be possible and not send commands faster than the interval, on sliding the first command gets sent after interval passed
  • like before, the value displayed is the one of the user's action, after a certain time has passed it changes to the item's state
  • both, the interval and the state display timeout are configurabe, updateInterval and delayStateDisplay, with defaults being 500ms and 2000ms, added as advanced configuration to both components
  • since oh-slider is practically the same thing, just displayed differently, I adjusted that to behave the same way (and because I wanted to be able to 'live'-dim my lamps 😉)
  • since the logic is the same for both components, I outsourced it to a mixin (slide-mixin.vue)

Let me know what you think.

@hubsif hubsif requested a review from a team as a code owner April 20, 2021 18:22
@relativeci
Copy link

relativeci bot commented Apr 20, 2021

Job #118: Bundle Size — 10.44MB (~-0.01%).

b12a665 vs 45b1533

Changed metrics (3/8)
Metric Current Baseline
Initial JS 1.6MB(+0.04%) 1.6MB
Cache Invalidation 21.31% 0%
Modules 1456(+0.07%) 1455
Changed assets by type (1/7)
            Current     Baseline
JS 8.14MB (~-0.01%) 8.14MB

View Job #118 report on app.relative-ci.com

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a lot of trial and error to make these controls work reasonably well, but obviously it was still far from perfect, and while I'm very careful to avoid regressions, I can't seem to fault the reworked implementation after initial testing. So I'd say it does seem a superior one indeed. The additonal parameters to control the debouncing is also a welcome addition.
Just a few remarks below:

@@ -0,0 +1,58 @@
<script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention is to have mixins in regular .js file, not .vue files, and thus this <script> tag, and the closing tag, are unnecessary.
Please rename the file to slide-mixin.js and remove those tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, not sure why I did that 🤔. Will correct, of course!

widget: OhSliderDefinition,
mounted () {
delete this.config.value

// f7-range inside of masonry can get rendered faulty, as the masonry changes its breakpoint layout after being rendered
// re-calculate the range slider after masonry is updated
setTimeout(() => {
this.$refs.rangeslider.f7Range.calcSize()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to this PR, but I just noticed the following error appears quite frequently referencing this line:

oh-slider.vue?08ef:24 Uncaught TypeError: Cannot read property 'f7Range' of undefined
    at eval (oh-slider.vue?08ef:24)

Perhaps this PR is a good opportunity to solve it as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. But as it seems, I can't reproduce that. I could just add an if ...f7Range..., but I'd prefer to test it. Can you provide a short YAML and your actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, added it blindly now. Please test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, we'll know soon enough if there are major regressions with this new implementation, but I don't any so far!

Fix oh-slider issue

Signed-off-by: Hubert Nusser <[email protected]>
@ghys ghys merged commit d586b77 into openhab:main May 16, 2021
@ghys ghys added this to the 3.1 milestone May 30, 2021
@ghys ghys added enhancement New feature or request main ui Main UI labels May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oh-knob-card
2 participants