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 value label text property #78

Closed
wants to merge 1 commit into from

Conversation

ryota765
Copy link

@ryota765 ryota765 commented Feb 5, 2022

There is a demand to use custom text for valueLabel, such as below.
slider-with-label-text

This revision enables to set custom text by passing string array.
multiSlider.valueLabelTexts = ["value1", "value2"]

Below are the points that can be considered

  • In this implementation, when the size of valueLabelTexts differs from the size of value, original text (which is the value) is displayed.
  • It might be better to rename valueLabel to a name such as thumbLabel, since the label will not always be a value.

@yonat
Copy link
Owner

yonat commented Feb 5, 2022

Thanks for the PR!

I wonder what is the use case for this?
Is it something you can't do with valueLabelFormatter?

@ryota765
Copy link
Author

ryota765 commented Feb 6, 2022

Thank you for the comment!

For example, you are making an app that gives you notification whether to bring an umbrella or a parasol according to the rain probability in the weather forecast data.
If the rain probability is higher than a certain value, the app notifies to bring an umbrella, and vice versa.

The user can set a threshold for the notification from a slider such as below.
slider-umbrella-parasol

In my understanding, valueLabelFormatter can express "formatted number" + "constant string", but cannot show custom string as in the above use case.

@yonat
Copy link
Owner

yonat commented Feb 6, 2022

Interesting: Without showing the actual numbers? I'm thinking something like:

slider.valueLabelFormatter.positivePrefix = "Umbrella: "
slider.valueLabelFormatter.negativePrefix = "Parasol: "

(Not trying to make life difficult, just want to make sure the added complexity is really necessary.)

@ryota765
Copy link
Author

ryota765 commented Feb 7, 2022

negativePrefix adds prefix when the value is negative and positivePrefix adds when the value is positive.
In the above example, rain probability is always positive (0 ~ 100%), so the prefix of all thumbs will become "Umbrella: ".

I don't want to show actual numbers in my case, since the value is an output from a machine learning model, which has no actual meaning such as probability. In this case, I thought tuning the threshold intuitively by not showing actual values are more user friendly.

just want to make sure the added complexity is really necessary

I agree with this thought.

@yonat
Copy link
Owner

yonat commented Feb 7, 2022

Okay, I think it may be an opportunity to generalize it a little. I put my idea in #80 - would you take a look and tell me what you think?

@yonat
Copy link
Owner

yonat commented Feb 8, 2022

Added valueLabelTextForThumb to 1.12.0, thanks for the idea!

@yonat yonat closed this Feb 8, 2022
@yonat
Copy link
Owner

yonat commented Oct 25, 2023

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Adding a feature to set custom text for value labels in a MultiSlider component.
  • 📝 PR summary: This PR introduces a new feature that allows users to set custom text for value labels in a MultiSlider component. The custom text can be set by passing a string array to the valueLabelTexts property. If the size of valueLabelTexts differs from the size of value, the original text (which is the value) is displayed.
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 2, because the PR is relatively small and straightforward, but it introduces a new feature which needs to be carefully reviewed.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and the feature is implemented in a clear and understandable way. However, it would be beneficial to add tests to ensure the new feature works as expected. Also, consider handling the case when the size of valueLabelTexts differs from the size of value more gracefully, perhaps by throwing an error or warning.

  • 🤖 Code feedback:

    • relevant file: Sources/MultiSlider+Internal.swift
      suggestion: Consider adding a guard statement to ensure valueLabelTexts and value have the same length. This will prevent unexpected behavior and make the code more robust. [important]
      relevant line: '+ if valueLabelTexts.count == thumbCount {'

    • relevant file: Sources/MultiSlider.swift
      suggestion: It would be better to handle the case when valueLabelTexts and thumbCount have different lengths. Currently, if they have different lengths, the labels are not updated. Consider throwing an error or warning in this case. [medium]
      relevant line: '+ if valueLabelTexts.count == thumbCount {'

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

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.

None yet

3 participants