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

Add SliderWithCallback component #938

Merged

Conversation

mikolajlubiak
Copy link
Contributor

I wanted to do some actions in real time when the slider value changes, not only update the reference value, so I made a little SliderWithCallback component

I wanted to do some actions in real time when the slider value changes, not only update the reference value, so I made a little `SliderWithCallback` component

Signed-off-by: Mikołaj Lubiak <lubiak@proton.me>
@mikolajlubiak
Copy link
Contributor Author

Example usage: https://github.com/mikolajlubiak/memory/

@ArthurSonzogni
Copy link
Owner

Thanks! This looks like a good thing to have.

Could you please extend the current SliderOption instead? This would avoid duplicating the current code.

@mikolajlubiak
Copy link
Contributor Author

Hi, done. I wasn't sure whether to initialize callback to empty lambda or nullptr, but nullptr seemed like a better option to me. What do you think?

@mikolajlubiak
Copy link
Contributor Author

@ArthurSonzogni friendly ping

@ArthurSonzogni
Copy link
Owner

Thanks! LGTM.

  • I renamed callback into on_change.
  • I added tests.
  • I fixed bugs where on_change was called multiple time per change.
  • I added the change to the CHANGELOG.

If anything looks wrong to you, please let me know, and we will maek the appropriate change.

@ArthurSonzogni ArthurSonzogni merged commit 99df1ac into ArthurSonzogni:main Oct 29, 2024
5 checks passed
@@ -127,8 +123,11 @@ class SliderBase : public ComponentBase {
OnUp();
}

SetValue(util::clamp(value_(), min_(), max_()));
if (old_value != value_()) {
this->value() = std::max(this->min(), std::min(this->max(), this->value()));
Copy link
Contributor Author

@mikolajlubiak mikolajlubiak Oct 29, 2024

Choose a reason for hiding this comment

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

Out of curiosity, why use this instead of std::clamp?

@mikolajlubiak
Copy link
Contributor Author

mikolajlubiak commented Oct 29, 2024

To get to the contributors list I would have to make more contributions, right?

@mikolajlubiak
Copy link
Contributor Author

mikolajlubiak commented Oct 29, 2024

Also would it be cool to add my memory game or media (video/gif/image) into colored, animated or static, ASCII art converter to the projects using FTXUI list or is it for more advanced projects?

@ArthurSonzogni
Copy link
Owner

To get to the contributors list I would have to make more contributions, right?

You are in the contributor list:
image

It might take a few hours for the displayed image to be invalidated and the new version to appear.

Also would it be cool to add my memory game or media (video/gif/image) into colored, animated or static, ASCII art converter to the projects using FTXUI list or is it for more advanced projects?

It would be cool. Do you want to submit another PR adding it?

@mikolajlubiak
Copy link
Contributor Author

Thanks, I made the PR: #946

std::max(this->min(), std::min(this->max(), this->value()));
Out of curiosity, why use this instead of std::clamp?

ArthurSonzogni added a commit that referenced this pull request Dec 26, 2024
Add SliderOption::on_change.

Useful to observe a change to the value.

Signed-off-by: Mikołaj Lubiak <lubiak@proton.me>
Co-authored-by: ArthurSonzogni <sonzogniarthur@gmail.com>
ArthurSonzogni added a commit that referenced this pull request Dec 26, 2024
Add SliderOption::on_change.

Useful to observe a change to the value.

Signed-off-by: Mikołaj Lubiak <lubiak@proton.me>
Co-authored-by: ArthurSonzogni <sonzogniarthur@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants