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 a third tint color to enhance the UI experience #241

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Monir-Shembesh
Copy link

After testing with only two tint colors the progress circle felt limited. For example, going from the color 'green' to the color 'red' at 100% produces a weird darkish 'green' color at 50%. Where as, some developers such as myself would like the 50% bar to be a yellow tint color. The solution is simple and straight forward, I have added a third tint color to enable a smoother color transition.

Reference: Traffic Light, credit card balance limit reached(green, yellow, red)

@talhaazhar
Copy link
Contributor

talhaazhar commented Mar 27, 2020

hey @Monir-Shembesh when I was making my changes I thought of this edge-case being super useful but was too lazy to add it. Wouldn't it be more beneficial to keep tintColorSecondary and just change it to an array? This way, if someone is looking to add 5 colors in the future, they can just reuse your code!

const arrayLength = tintColorSecondary.length;
let inputRange = Array.from({length: arrayLength + 1}).map(
   (currElement, i) => i * (100 / arrayLength)
);
let outputRange = tintColorSecondary.unshift(this.props.tintColor);

@Monir-Shembesh
Copy link
Author

Monir-Shembesh commented Mar 28, 2020

Hi @talhaazhar check this implementation then?

animateColor() {

    if (!this.props.multiColor) {
      return this.props.singleColor
    }

    const colorsArry = this.props.multiColor.length;
    const inputRange = Array.from({ length: colorsArry }).map(
      (currElement, index) => index * (100 / colorsArry));

    const tintAnimation = this.state.fillAnimation.interpolate({
      inputRange,
      outputRange: this.props.multiColor
    })

    return tintAnimation

  }

so what I am aiming for here is instead of using tint color and then tintColorSecondary to achieve more than one color effect, we give the developer the option to either choose a single color or two or more colors.

before:

<AnimatedCircularProgress
                        size={width(50)}
                        width={20}
                        fill={50}
                        rotation={0}
                        lineCap={'round'}
                        tintColor={'red'}
                        tintColorSecondary={['blue', 'Yellow']}
/>

after:

<AnimatedCircularProgress
                        size={width(50)}
                        width={20}
                        fill={50}
                        rotation={0}
                        lineCap={'round'}
                        singleColor={'red'}
                        //or multiColor={['blue', 'Yellow']}
/>

The issue now is how to tell the developers that multi color prop will need minimum of two colors otherwise output range will throw an error

@talhaazhar
Copy link
Contributor

Hey @Monir-Shembesh, this looks great. However, appending tintColor to the start of the array fixes this (instead of expecting the user to add it):

let outputRange = tintColorSecondary.unshift(this.props.tintColor);

This way the tintColor is always used as the first one. Also, you could rename multiColor to something like tintTransitionColors, it might sound more intuitive.

Although, the real solution would be to remove tintColor all together and just have the array color option. I don't know if that'd be too dramatic of a change for old users though.

@Monir-Shembesh
Copy link
Author

yh unshift will solve the issue but i only wanted to keep the multiColors array. in my local repo i already remove tintColor as i displayed above. The users dont need to update to the latest package version if they dont need multiColors but if they do they can update and we can then leave a note in the README file that illustrate this change. what do you think? @talhaazhar

@talhaazhar
Copy link
Contributor

I already agree with the idea of removing tintColor completely since that is the correct way to handle this update, however, the final review depends on the Repo Owner. Although it'd be nice to rename the multiColor to tintTransitionColors.

@Monir-Shembesh
Copy link
Author

Yh I agree with tintTransitionColors, I will write something up in a bit further improve the above code and send a pull request. @talhaazhar @bartgryszko

@aboveyunhai
Copy link
Contributor

Hey @Monir-Shembesh, this looks great. However, appending tintColor to the start of the array fixes this (instead of expecting the user to add it):

let outputRange = tintColorSecondary.unshift(this.props.tintColor);

This way the tintColor is always used as the first one. Also, you could rename multiColor to something like tintTransitionColors, it might sound more intuitive.

Although, the real solution would be to remove tintColor all together and just have the array color option. I don't know if that'd be too dramatic of a change for old users though.

Neat thought! My idea would be completely removing the secondaryColor and make the tintColor can accept either a single string/array of one string for single element, and array of string for multiple. But I'm not sure how it would affect the library dramatically.

And parse the transition array [0, 50, 100] dynamically unless the user provide the array manually.

For instance, without providing the array and tintColor = {["blue", "red", "green", "orange"]}, the transition array might be something like [0, 33 , 66, 100] . And you can always provide your own array like [0, 25, 50, 100] to adjust the effect but it must follow certain standard in order to use it correctly. There are some edge cases need to be handle also.

@Monir-Shembesh
Copy link
Author

Monir-Shembesh commented Apr 7, 2020

Hey @Monir-Shembesh, this looks great. However, appending tintColor to the start of the array fixes this (instead of expecting the user to add it):
let outputRange = tintColorSecondary.unshift(this.props.tintColor);
This way the tintColor is always used as the first one. Also, you could rename multiColor to something like tintTransitionColors, it might sound more intuitive.
Although, the real solution would be to remove tintColor all together and just have the array color option. I don't know if that'd be too dramatic of a change for old users though.

Neat thought! My idea would be completely removing the secondaryColor and make the tintColor can accept either a single string/array of one string for single element, and array of string for multiple. But I'm not sure how it would affect the library dramatically.

And parse the transition array [0, 50, 100] dynamically unless the user provide the array manually.

For instance, without providing the array and tintColor = {["blue", "red", "green", "orange"]}, the transition array might be something like [0, 33 , 66, 100] . And you can always provide your own array like [0, 25, 50, 100] to adjust the effect but it must follow certain standard in order to use it correctly. There are some edge cases need to be handle also.

Hi, i was going to cook some code for this and I actually did but was never able to push it as I got bombarded with work. Check my final solution i came up with to this matter.

animateColor() {

    //for default color value

    if (!this.props.tintTransitionColors) {
      return 'black'
    }

    // to counter the output range issue, we check array length. if it is 1 we return the 0 index of that 
   //array. and now it supports 1 color only if the user wanted.

    if (this.props.tintTransitionColors.length === 1) {
      return this.props.tintTransitionColors[0]
    }

    // here I take and display the colors equally within the wheel. if its 4 colors and the wheel is 100
   // then it will be [0, 25, 50, 75, 100]. if its five colors then it will automatically adapt. 

  

    const colorsArry = this.props.tintTransitionColors.length;
    const inputRange = Array.from({ length: colorsArry }).map(
      (currElement, index) => index * (this.props.circleEndingValue / colorsArry));

//how ever notice that there is a new variable i added called "circleEndingValue" right?
  // well i didnt want my circle to finish at 100, so i change the code structure to accept the value i 
  //want my circle to close at. this is another feature I added which we can send a pull request for 
 //later

    const tintAnimation = this.state.fillAnimation.interpolate({
      inputRange,
      outputRange: this.props.tintTransitionColors
    })

    return tintAnimation

  }

If you guys accept this solution then let me know asap and i will send a pull request and document the README file if any changes are needed. @aboveyunhai @talhaazhar

@talhaazhar
Copy link
Contributor

It looks great. Just make sure to search tintColor in the entire repo and remove it elsewhere!

@aboveyunhai
Copy link
Contributor

aboveyunhai commented Apr 8, 2020

Using tintTransitionColors instead of tintColor may make better sense literally if it "intentionally" uses the color transition functionality, but it's primarily used as a "tintColor". And in fact "tintColor " can contain "tintTransitionColors" in literal. I personally disagree with removing and changing tintColor to other names (at least for now unless it's necessary for the functional change in the future).

Most importantly, tintColor is generally set by ppl who uses this library. "Side" change (this PR is nice but a not major change in my opinion) without considering the consequence of will break many apps and cause troubles, unless there was already a solution to make smooth update but I missed out from the discussion.

@Monir-Shembesh
Copy link
Author

@aboveyunhai I see your point of view but let me run you through some points.

Documentation is key, tintTransitionColors can contain 1 color or multiple colors thats what we are aiming for here. Not just because the prop name is plural means they must have 2 or more colors.

If you add a literal to tintColor you are making the code more comlicated to the end user/developer who will use this library so KISS (keep it simple stupid) but most importantly functions well as well as being robust. Not to mention that its basically the same point you made where we use a singular noun then pass in multiple colors.

Lastly, this current change will not break on of the apps that are using the current version unless they are directly pulling the repo in their package.json file or they updated to the latest version after the merge. Thus, anyone using the current version will stay safe.

I think the final decision will come down to what the repo owner @bartgryszko sees most suitable. But we are a community and if you feel like you have a better solution please help us make things better :D.

@Monir-Shembesh
Copy link
Author

Hi all, please do no merge this branch yet i am going to make change the code to add an end percentage. Will explain more either TMW or After

@fareedagha
Copy link

hello #Monir did you change the code that you mentioned above?

@Monir-Shembesh
Copy link
Author

@fareedagha sorry for late response. I will be sending the pull request later at night Today.

@fareedagha
Copy link

@Monir-Shembesh thanks, please let me know once you done with pull request

@hamol355
Copy link

hamol355 commented Sep 29, 2022

@Monir-Shembesh Is this feature abandoned?

@theyanniss23002
Copy link

Guys, we urgently need the ability to multi colors

@markusl
Copy link
Collaborator

markusl commented Jan 20, 2023

Guys, we urgently need the ability to multi colors

Would you be able to finish the PR?

@ManigandanRaamanathan
Copy link

Is this feature abandoned?

@markusl
Copy link
Collaborator

markusl commented Jan 21, 2024

Is this feature abandoned?

@ManigandanRaamanathan if you could finish the PR, fixing the merge issues it can be merged :)

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.

9 participants