Skip to content

Conversation

@JonasBa
Copy link
Member

@JonasBa JonasBa commented Jan 3, 2022

Add some utils for keeping the canvas properly resized and add the main rect class that we use in order to represent the flamegraph's frames.

@JonasBa JonasBa requested a review from a team as a code owner January 3, 2022 13:55
@JonasBa JonasBa requested a review from a team January 3, 2022 13:56
Copy link
Member

Choose a reason for hiding this comment

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

lowercase type leaves me sad

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agree :(

Comment on lines 214 to 220
Copy link
Member

Choose a reason for hiding this comment

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

how do you feel about this?

Suggested change
// it's easier to display a matrix as a 3x3 array. WebGl matrices are row first and not column first
// https://webglfundamentals.org/webgl/lessons/webgl-matrix-vs-math.html
// prettier-ignore
return mat3.fromValues(
this.width, 0, 0,
0, this.height, 0,
this.x, this.y, 1
)
const {width: w, height: w, x, y} = this;
// it's easier to display a matrix as a 3x3 array. WebGl matrices are row first and not column first
// https://webglfundamentals.org/webgl/lessons/webgl-matrix-vs-math.html
// prettier-ignore
return mat3.fromValues(
w, 0, 0,
0, h, 0,
x, y, 1
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, looks much more readable :) (I'm not a fan of the extra allocations, but who cares)

Copy link
Member

Choose a reason for hiding this comment

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

damn I don't know why our eslint isn't catching this. I thought I specifically fixed this here getsentry/eslint-config-sentry#117

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, indeed. This should have been caught, I dont see an error in vscode either.

Copy link
Member Author

Choose a reason for hiding this comment

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

@evanpurkhiser I think the config we use for this codebase actually never extends config/rules/base (or it overrides it somewhere).

In other words, if I manually set .eslintrc.js to

extends: ['sentry-app/strict', 'sentry/rules/base'],

Then I get Expected { after 'if' condition.eslintcurly, but if I set it to

extends: ['sentry/rules/base', 'sentry-app/strict'],

The error does not show, which makes me think that we are overriding the rule here somehow, or that sentry-app/strict just never extends it. Maybe something for @billyvg to investigate?

Copy link
Member

Choose a reason for hiding this comment

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

Whoa whoa, good findings

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'll take a look

Copy link
Member

@billyvg billyvg Jan 4, 2022

Choose a reason for hiding this comment

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

@JonasBa @evanpurkhiser Ah it's because of the prettier plugin, we'll need to override it in sentry-app (or wherever we use the plugin) https://github.com/prettier/eslint-config-prettier/tree/79db534f6afc0be1c9b1a6c7c3fdf825cbe52827#special-rules

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice find @billyvg, thanks for investigating!

@JonasBa
Copy link
Member Author

JonasBa commented Jan 5, 2022

Rebasing again because acceptance tests timeout

@JonasBa JonasBa deleted the jb/profiling/gl-utils branch January 7, 2022 10:47
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants