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

feat: add int knob #942

Merged
merged 10 commits into from Oct 10, 2023
Merged

Conversation

Mastersam07
Copy link
Contributor

@Mastersam07 Mastersam07 commented Oct 6, 2023

  • Add Int number knob.
  • Add generic NumXField.
  • IntXField and DoubleXField extending generic NumXField

List of issues which are fixed by the PR

#430

Screenshots

int_knob.mov

Checklist

  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making].
  • All existing and new tests are passing.

If you need help, consider asking for advice on Discord.

hide 'IntKnob' from package public API
@Mastersam07
Copy link
Contributor Author

Hi @YoussefRaafatNasry can you please take a look/review?

@YoussefRaafatNasry YoussefRaafatNasry linked an issue Oct 8, 2023 that may be closed by this pull request
Copy link
Member

@YoussefRaafatNasry YoussefRaafatNasry left a comment

Choose a reason for hiding this comment

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

While this implementation is well-crafted, but we are looking into feature-parity between double and int knobs (i.e knobs.int.slider, knobs.int.input, etc.).

This will result into a lot of duplicated code between the DoubleXField and IntXField, so can you explore the possibility of creating a generic NumSilderField<T extends num> and NumInputField<T extends num> that can be used for both int and double knobs?

@Mastersam07
Copy link
Contributor Author

Mastersam07 commented Oct 8, 2023

While this implementation is well-crafted, but we are looking into feature-parity between double and int knobs (i.e knobs.int.slider, knobs.int.input, etc.).

This will result into a lot of duplicated code between the DoubleXField and IntXField, so can you explore the possibility of creating a generic NumSilderField<T extends num> and NumInputField<T extends num> that can be used for both int and double knobs?

Does this mean we end up with only NumXField or we still maintain DoubleXField and IntXField but with shared code in the NumXField?

@YoussefRaafatNasry
Copy link
Member

Does this mean we end up with only NumXField or we still maintain DoubleXField and IntXField but with shared code in the NumXField?

@Mastersam07 We will maintain all 6 classes (NumSliderField, NumInputField,DoubleSliderField, DoubleInputField, IntSliderField, IntInputField). The base class will have all the shared logic, the child classes will only extend it without having too much code to maintain.

@Mastersam07
Copy link
Contributor Author

Does this mean we end up with only NumXField or we still maintain DoubleXField and IntXField but with shared code in the NumXField?

@Mastersam07 We will maintain all 6 classes (NumSliderField, NumInputField,DoubleSliderField, DoubleInputField, IntSliderField, IntInputField). The base class will have all the shared logic, the child classes will only extend it without having too much code to maintain.

Alright. Made the changes.

remove unneccessary type definition
@Mastersam07
Copy link
Contributor Author

@YoussefRaafatNasry please review

@YoussefRaafatNasry
Copy link
Member

@Mastersam07 Apology for this taking time, but I have it in my queue, no need to worry about it.
We have high activity for Hacktoberfest, along with our other tasks, so it might take a day or two to review (excluding weekends)

Copy link
Contributor Author

@Mastersam07 Mastersam07 left a comment

Choose a reason for hiding this comment

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

@YoussefRaafatNasry the conflict is due to places like this that were specified as type int. There is no conflict with double because no variable/args is specified as type double.

@YoussefRaafatNasry
Copy link
Member

YoussefRaafatNasry commented Oct 10, 2023

@Mastersam07 makes sense.
Let's go with integer / integerOrNull.
I feel like we are also going to run into the same problem later with double, but let's wait and see.

@YoussefRaafatNasry
Copy link
Member

@Mastersam07 Nevermind the last comment. I have came up with an idea as a workaround, because I am more leaning towards the int name over integer.

You can add a typedef at the top of the file as follows:

typedef $int = int;

Then we name IntKnobsBuilder as int
and any int parameter as $int

@Mastersam07
Copy link
Contributor Author

@Mastersam07 Nevermind the last comment. I have came up with an idea as a workaround, because I am more leaning towards the int name over integer.

You can add a typedef at the top of the file as follows:

typedef $int = int;

Then we name IntKnobsBuilder as int and any int parameter as $int

Alright. Made the changes.

Copy link
Member

@YoussefRaafatNasry YoussefRaafatNasry left a comment

Choose a reason for hiding this comment

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

Another interesting PR, thanks a lot @Mastersam07 for improving Widgetbook! 💙

@YoussefRaafatNasry YoussefRaafatNasry merged commit b0c453f into widgetbook:main Oct 10, 2023
5 checks passed
@Mastersam07 Mastersam07 deleted the feat/int_knob branch October 10, 2023 16:42
@Mastersam07 Mastersam07 restored the feat/int_knob branch October 10, 2023 16:42
@Mastersam07 Mastersam07 deleted the feat/int_knob branch October 10, 2023 16:42
@Mastersam07 Mastersam07 restored the feat/int_knob branch October 10, 2023 16:42
@Mastersam07 Mastersam07 deleted the feat/int_knob branch October 10, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Int Number Knob
2 participants