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

Change eval methods for multiple feature #1376

Merged

Conversation

elenatorro
Copy link
Contributor

Related issue #1348

This PR adds:

  • Refactor binary operation expressions
  • Fix value and eval for binary operation expressions
  • Add getLegendData for binary operation expressions

This means that we can also have Bivariate legends in Airship and CARTOframes.

Screenshot 2019-06-24 at 16 51 07

Nice to have:

  • Better and public example of Bivariate legend (different task)

@elenatorro elenatorro requested a review from VictorVelarde June 24, 2019 14:56
let content = '';

legends.data.forEach((legend) => {
const colorHex = rgbToHex(legend.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

@elenatorro I think we can now remove this rgbToHex thing, and just usa rgba as in feature-and-legend-opacity.html example

Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably move this 'debug' example to public ones, what do you think? Or maybe we should do a small blog post?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally! I'd like to have this in a separate task (since this one is a bit big), and it'd be great to have more than one example (but not too much, I mean, like two or three). At leas this one and another one that combines with and color properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. We can keep the rgbToHex if it is just a debug example.

@@ -1,4 +1,11 @@
import { And, Or, Equals, NotEquals, LessThan, LessThanOrEqualTo, GreaterThan, GreaterThanOrEqualTo } from '../renderer/viz/expressions/binary';
import GreaterThan from '../renderer/viz/expressions/binary/GreaterThan';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this small refactor here? Was it maybe based on some tooling?

Copy link
Contributor

@VictorVelarde VictorVelarde Jun 27, 2019

Choose a reason for hiding this comment

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

Oh, I see it now... you deleted the binary.js, much better

});

describe('NUMBER_AND_COLOR_TO_COLOR', () => {
testValue('mul', 10, s.rgba(255, 15, 12, 1), { r: 10, g: 1, b: 0, a: 1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

When does this NUMBER * COLOR is useful? do you know any example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, no, but it's been there for a while and I think there was an example. I'll do a bit of research.

});
}

describe('mul', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we split also the tests into different files? Especially now that we have 1 file per operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I agree, I'll change that :)

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM, just some small comments, and a question about certain 'number' & 'color' operations

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM, just some small comments, and a question about certain 'number' & 'color' operations

Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

LGTM, just some small comments, and a question about certain 'number' & 'color' operations

@VictorVelarde VictorVelarde self-requested a review June 28, 2019 18:48
Copy link
Contributor

@VictorVelarde VictorVelarde left a comment

Choose a reason for hiding this comment

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

🚀

@elenatorro elenatorro merged commit 644e4af into develop Jul 12, 2019
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