Skip to content

Conversation

@aibcmars
Copy link
Contributor

Code mutualization

@aibcmars aibcmars added the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label Apr 29, 2020
@aibcmars
Copy link
Contributor Author

Depends on #222

@aibcmars aibcmars added the enhancement New feature or request label Apr 29, 2020
@aibcmars aibcmars changed the title [INFRA] improved rendering for exclusive gateway [FEAT] improved rendering for exclusive gateway Apr 29, 2020
@tbouffard tbouffard added the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label Apr 30, 2020
@aibcmars aibcmars removed the rebase needed 💥 Pull request that must be rebased on the latest master commit prior being reviewed or merged label May 6, 2020
@aibcmars aibcmars force-pushed the 197_improved_scaleFactorCanvas branch from 7fe7277 to bc69a6d Compare May 6, 2020 13:03
@aibcmars aibcmars removed the depends on another PR ⚠️ Pull request depending on another one. The depending must be merged first label May 6, 2020
@aibcmars aibcmars force-pushed the 197_improved_scaleFactorCanvas branch from bc69a6d to aec30da Compare May 6, 2020 15:10
@tbouffard
Copy link
Member

Render master fae688d
01_master_fae688d

New render aec30da
02_pr226_aec30da

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

LGTM
However, some refactoring as needed IMHO because this PR doesn't fully covered what I had in mind with #222 (comment)

  • scaling is still explicitly present in code outside of the canvas wrapper
  • it is not possible to scale on x and y with different values
  • translation is done globally which leads to side effects

So, I have create a new issue to track this missing technical requirement, see #247


c.fillAndStroke();
const canvas = this.configureCanvasForIcon(c, w, h, 0);
this.translateToStartingIconPosition(c, x, y, w, h);
Copy link
Member

@tbouffard tbouffard May 6, 2020

Choose a reason for hiding this comment

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

⚠️ this should be done in the canvas wrapper, let's manage this in another refactoring PR

@aibcmars aibcmars merged commit e606296 into master May 6, 2020
@aibcmars aibcmars deleted the 197_improved_scaleFactorCanvas branch May 6, 2020 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants