Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Consolidate code for determining the pixel ratio #3088

Merged
merged 3 commits into from
Jun 13, 2017

Conversation

wimrijnders
Copy link
Contributor

A refactoring of the 'can not unsee' variety.

Consolidate code for determining the pixel ratio in CanvasRenderer, with the same code present in Canvas.

bradh
bradh previously approved these changes May 22, 2017
Copy link
Contributor

@bradh bradh left a comment

Choose a reason for hiding this comment

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

The naming could be more obvious (maybe calculatePixelRatio / _updatePixelRatio rather than _getPixelRatio / _setPixelRatio), but that is aesthetics, and could well be another change.

@wimrijnders
Copy link
Contributor Author

wimrijnders commented May 22, 2017

@bradh Yeah, I agree. Some more thought could be put in combining more related common functionality. E.g. in all cases but one, the pixel ratio is used to set the transform in a context - which invariably comes from the Canvas instance, so why should CanvasRenderer be doing it? Things like that.

But current is a quick fix to remove the hurt in my eyes. Any more works requires non-zero brainpower.

…as.pixelRatio is used.

- consolidated ctx.transform() calls in Canvas
- Added/edited commenting (also TODO's)
- Added Canvas.getContext()
@wimrijnders
Copy link
Contributor Author

You can review this PR now BTW. I've done enough refactoring for pixel ratio.

@yotamberk yotamberk merged commit 98c0922 into almende:develop Jun 13, 2017
@wimrijnders wimrijnders deleted the refactorGetPixelRatio branch June 13, 2017 19:14
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
* Consolidate code for determining the pixel ratio

* - Removed local param  'pixerRatio' from CanvasRenderer. Now only canvas.pixelRatio is used.
- consolidated ctx.transform() calls in Canvas
- Added/edited commenting (also TODO's)
- Added Canvas.getContext()
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.

3 participants