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

[FIX] discuss: fix slider values in call settings #151028

Closed

Conversation

ThanhDodeurOdoo
Copy link
Contributor

@ThanhDodeurOdoo ThanhDodeurOdoo commented Jan 25, 2024

  • Adds text values for the call settings sliders.
  • Replaces onChange events with onInput events so that the values
    respect the position of the sliders.
  • Debounces some functions to handle the increased amount of calls
    due to the swap to the onInput listener.
  • Changes the default value of voiceActiveDuration from 0 to 200
    to match the minimum value of the input.

@robodoo
Copy link
Contributor

robodoo commented Jan 25, 2024

@C3POdoo C3POdoo added the RD research & development, internal work label Jan 25, 2024
@ThanhDodeurOdoo
Copy link
Contributor Author

cc @zel-odoo

@zel-odoo
Copy link
Contributor

cc @zel-odoo

thanks for reminding me. is this going to be done in 17.0 soon? I can wait for rebase either make the change in master in my PR if it is gonna merged soon.

@ThanhDodeurOdoo
Copy link
Contributor Author

@zel-odoo since you are removing the file and moving the code, you may just do the changes to your new file (not sure if rebase will be smart enough), but wait that the PR is reviewed, more values display may be added.

@ThanhDodeurOdoo ThanhDodeurOdoo marked this pull request as ready for review January 25, 2024 10:21
@zel-odoo
Copy link
Contributor

@ThanhDodeurOdoo OK, then I will directly change the code according to your change.

@C3POdoo C3POdoo requested review from a team January 25, 2024 11:00
@ThanhDodeurOdoo ThanhDodeurOdoo force-pushed the 17.0-input-values-fix-tso branch 4 times, most recently from e3c695e to 136d9d9 Compare January 26, 2024 12:33
@ThanhDodeurOdoo ThanhDodeurOdoo changed the title [FIX] discuss: show relevant slider values in call settings [FIX] discuss: fix slider values in call settings Jan 26, 2024
@ThanhDodeurOdoo ThanhDodeurOdoo force-pushed the 17.0-input-values-fix-tso branch 3 times, most recently from 6744300 to d7c334c Compare January 30, 2024 11:51
@phenix-factory
Copy link
Contributor

@ThanhDodeurOdoo You have a bunch of sliders and @zel-odoo is also adding a bunch of them based on yours. Maybe this could be a Slider component generic component?

@zel-odoo
Copy link
Contributor

@ThanhDodeurOdoo You have a bunch of sliders and @zel-odoo is also adding a bunch of them based on yours. Maybe this could be a Slider component generic component?

image
Something like this

@ThanhDodeurOdoo
Copy link
Contributor Author

@phenix-factory yes that's probably the goal for master, we will need it when doing the slider with visual feedback for the voice threshold

@ThanhDodeurOdoo ThanhDodeurOdoo force-pushed the 17.0-input-values-fix-tso branch 3 times, most recently from e12632c to a776d18 Compare February 1, 2024 09:00
Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

<input class="flex-grow-2 form-range" type="range" min="0" max="20" step="1" t-att-value="userSettings.edgeBlurAmount" t-on-change="onChangeEdgeBlurAmount"/>
<div class="d-flex w-100 align-items-center">
<input class="flex-grow-2 form-range" type="range" min="0" max="20" step="1" t-att-value="userSettings.edgeBlurAmount" t-on-input="onChangeEdgeBlurAmount"/>
<span class="p-1 w-50 text-end"><t t-out="Math.floor(userSettings.edgeBlurAmount * 5)"/>%</span>
Copy link
Contributor

@zel-odoo zel-odoo Feb 1, 2024

Choose a reason for hiding this comment

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

i would suggest using width: 6ch in .scss files. means three digits + % and ` character for buffering... 25% width is way too much and it is displaying well in my PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zel-odoo yes it felt a bit too big, but not enough to make me want to create a new scss file just for that in a fix, if you have more finely tuned dimensions in your PR for master, i think that it's good

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, then I will have it at my PR

@zel-odoo
Copy link
Contributor

do we still want to have this PR? Should I apply those changes into mine ( configuration menu) ?

@ThanhDodeurOdoo
Copy link
Contributor Author

@zel-odoo I will rebase it, I thought it was merged

* Adds text values for the call settings sliders.
* Replaces `onChange` events with `onInput` events so that the values
 respect the position of the sliders.
* Debounces some functions to handle the increased amount of calls
due to the swap to the `onInput` listener.
* Changes the default value of `voiceActiveDuration` from `0` to `200`
to match the minimum value of the input.
Copy link
Contributor

@seb-odoo seb-odoo left a comment

Choose a reason for hiding this comment

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

@robodoo r+ delegate+

@fw-bot
Copy link
Contributor

fw-bot commented Apr 1, 2024

@ThanhDodeurOdoo @seb-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

1 similar comment
@fw-bot
Copy link
Contributor

fw-bot commented Apr 2, 2024

@ThanhDodeurOdoo @seb-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Contributor

fw-bot commented Apr 3, 2024

@ThanhDodeurOdoo @seb-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Contributor

fw-bot commented Apr 4, 2024

@ThanhDodeurOdoo @seb-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

1 similar comment
@fw-bot
Copy link
Contributor

fw-bot commented Apr 5, 2024

@ThanhDodeurOdoo @seb-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants