Support hex values in outputRange #3177

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@dralletje
Contributor

dralletje commented Oct 1, 2015

  1. Should I add an example of this to the UIExplorer Animation example?
  2. Should I support mixing hex and rgba values in outputRange? It is possible with this implementation, but may cause confusion
@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 1, 2015

Contributor

fixes #1905 🙊

Contributor

dralletje commented Oct 1, 2015

fixes #1905 🙊

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 1, 2015

Contributor

Working on tests, sorry I forgot to add them!

Contributor

dralletje commented Oct 1, 2015

Working on tests, sorry I forgot to add them!

Libraries/Animated/src/Interpolation.js
+ input: string
+): string {
+ // Expand shorthand form (e.g. "03F") to full form (e.g. "0033FF")
+ var shorthandRegex = /^#?([a-f\d])([a-f\d])([a-f\d])$/i;

This comment has been minimized.

@ide

ide Oct 1, 2015

Collaborator

Use {3} for brevity
scratch that, the quantifier only gets the last capturing group

@ide

ide Oct 1, 2015

Collaborator

Use {3} for brevity
scratch that, the quantifier only gets the last capturing group

This comment has been minimized.

@dralletje

dralletje Oct 1, 2015

Contributor

Yeah indeed, wanted to use {3} but then found that out too 😝

@dralletje

dralletje Oct 1, 2015

Contributor

Yeah indeed, wanted to use {3} but then found that out too 😝

Libraries/Animated/src/Interpolation.js
+ // Expand shorthand form (e.g. "03F") to full form (e.g. "0033FF")
+ var shorthandRegex = /^#?([a-f\d])([a-f\d])([a-f\d])$/i;
+ var hex = input.replace(shorthandRegex, function(m, r, g, b) {
+ return r + r + g + g + b + b;

This comment has been minimized.

@ide

ide Oct 1, 2015

Collaborator

2 spaces for indentation

@ide

ide Oct 1, 2015

Collaborator

2 spaces for indentation

Libraries/Animated/src/Interpolation.js
@@ -178,6 +194,12 @@ function createInterpolationFromStringOutputRange(
): (input: number) => string {
var outputRange: Array<string> = (config.outputRange: any);
invariant(outputRange.length >= 2, 'Bad output range');
+
+ // Convert hex values into their rgba(r,g,b,1) equivalent
+ if (outputRange[0].match(hexShapeRegex)) {

This comment has been minimized.

@ide

ide Oct 1, 2015

Collaborator

I think it'd be good and less surprising to support a mix of rgb and hex.

@ide

ide Oct 1, 2015

Collaborator

I think it'd be good and less surprising to support a mix of rgb and hex.

+ expect(() => Interpolation.create({
+ inputRange: [0, 1],
+ outputRange: ['#FF00AB', 'rgb(50, 150, 250, 1)'],
+ })).toThrow();

This comment has been minimized.

@ide

ide Oct 1, 2015

Collaborator

...and then have this not throw

@ide

ide Oct 1, 2015

Collaborator

...and then have this not throw

Libraries/Animated/src/Interpolation.js
+
+ var longHexRegex = /^#([a-f\d]{2})([a-f\d]{2})([a-f\d]{2})$/i;
+ return hex.replace(longHexRegex, (m, r, g, b) =>
+ `rgba(${[r,g,b].map(x => parseInt(x, 16)).join(', ')}, 1)`

This comment has been minimized.

@ide

ide Oct 3, 2015

Collaborator

spaces after the commas

@ide

ide Oct 3, 2015

Collaborator

spaces after the commas

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 3, 2015

Collaborator

Thanks for adding tests too!

Collaborator

ide commented Oct 3, 2015

Thanks for adding tests too!

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 3, 2015

Contributor

Could you use tinycolor instead so that we can get all the colors supported including named ones like "blue" and "red" and we don't have to duplicate color parsing logic?

Then it's going to be great, thanks for doing that!

Contributor

vjeux commented Oct 3, 2015

Could you use tinycolor instead so that we can get all the colors supported including named ones like "blue" and "red" and we don't have to duplicate color parsing logic?

Then it's going to be great, thanks for doing that!

@dralletje

This comment has been minimized.

Show comment
Hide comment
@dralletje

dralletje Oct 4, 2015

Contributor

Not sure if this breaks for anyone, because it could be that some inputs not get converted to colors. Yet, I've added it! :-)

Contributor

dralletje commented Oct 4, 2015

Not sure if this breaks for anyone, because it could be that some inputs not get converted to colors. Yet, I've added it! :-)

Libraries/Animated/src/Interpolation.js
+ input: string
+): string {
+ var color = tinycolor(input);
+ var {r, g, b, a} = color.toRgb();

This comment has been minimized.

@vjeux

vjeux Oct 4, 2015

Contributor

Can you only declare the variable if it is valid. Otherwise calling toRGB is not safe

@vjeux

vjeux Oct 4, 2015

Contributor

Can you only declare the variable if it is valid. Otherwise calling toRGB is not safe

This comment has been minimized.

@dralletje

dralletje Oct 4, 2015

Contributor

Not safe? Tinycolor default to black, so it won't crash!
What do you mean by not safe?
(I'll change it anyway ^^)

@dralletje

dralletje Oct 4, 2015

Contributor

Not safe? Tinycolor default to black, so it won't crash!
What do you mean by not safe?
(I'll change it anyway ^^)

This comment has been minimized.

@vjeux

vjeux Oct 4, 2015

Contributor

Calling toRBG() on an invalid color is not safe in the sense that you're going to get an unexpected result. Either a crash or some arbitrary value.

In this case you know what it does because you dug in tinycolor implementation to figure it out. But the next person that's going to modify this function will see that r, g and b are defined and will use them even though it will not make sense.

In isolation this isn't dramatic, but as your codebase grows and there are unsafe accesses like this all over this is going to cause real bugs and worse, people are going to do crazy hacks to work around that.

A not too far fetched example would be that you somehow return rbg without the isvalid information in a function that's widely used. I can bet money that there will be some code that checks if rgb is black and then find the original string and do === 'black' to make sure the user really put black and not some random variable.

@vjeux

vjeux Oct 4, 2015

Contributor

Calling toRBG() on an invalid color is not safe in the sense that you're going to get an unexpected result. Either a crash or some arbitrary value.

In this case you know what it does because you dug in tinycolor implementation to figure it out. But the next person that's going to modify this function will see that r, g and b are defined and will use them even though it will not make sense.

In isolation this isn't dramatic, but as your codebase grows and there are unsafe accesses like this all over this is going to cause real bugs and worse, people are going to do crazy hacks to work around that.

A not too far fetched example would be that you somehow return rbg without the isvalid information in a function that's widely used. I can bet money that there will be some code that checks if rgb is black and then find the original string and do === 'black' to make sure the user really put black and not some random variable.

Libraries/Animated/src/Interpolation.js
+ var color = tinycolor(input);
+ var {r, g, b, a} = color.toRgb();
+ return color.isValid()
+ ? `rgba(${r}, ${g}, ${b}, ${a === undefined ? 1 : a})`

This comment has been minimized.

@vjeux

vjeux Oct 4, 2015

Contributor

Nit: can you use an if statement and return.

@vjeux

vjeux Oct 4, 2015

Contributor

Nit: can you use an if statement and return.

+ it('should work with output ranges with mixed hex and rgba strings', () => {
+ var interpolation = Interpolation.create({
+ inputRange: [0, 1],
+ outputRange: ['rgba(100, 120, 140, .5)', '#87FC70'],

This comment has been minimized.

@vjeux

vjeux Oct 4, 2015

Contributor

So good!

@vjeux

vjeux Oct 4, 2015

Contributor

So good!

This comment has been minimized.

@brentvatne

brentvatne Oct 4, 2015

Collaborator

👍

@brentvatne

brentvatne Oct 4, 2015

Collaborator

👍

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 4, 2015

Contributor

cc @javache, when are you going to land your diff it's going to slightly conflict with this one

Contributor

vjeux commented Oct 4, 2015

cc @javache, when are you going to land your diff it's going to slightly conflict with this one

@vjeux

This comment has been minimized.

Show comment
Hide comment
Contributor

vjeux commented Oct 4, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@ghost ghost closed this in 463b072 Oct 4, 2015

MattFoley added a commit to skillz/react-native that referenced this pull request Nov 9, 2015

Support hex values in outputRange
Summary: 1. Should I add an example of this to the UIExplorer Animation example?

2. Should I support mixing hex and rgba values in outputRange? It is possible with this implementation, but may cause confusion
Closes facebook#3177

Reviewed By: @​svcscm

Differential Revision: D2506947

Pulled By: @vjeux

Crash-- added a commit to Crash--/react-native that referenced this pull request Dec 24, 2015

Support hex values in outputRange
Summary: 1. Should I add an example of this to the UIExplorer Animation example?

2. Should I support mixing hex and rgba values in outputRange? It is possible with this implementation, but may cause confusion
Closes facebook#3177

Reviewed By: @​svcscm

Differential Revision: D2506947

Pulled By: @vjeux

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment