Skip to content

cleanup: remove useCanvasTransformSync composables. #5742

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 24, 2025
Merged

cleanup: remove useCanvasTransformSync composables. #5742

merged 3 commits into from
Sep 24, 2025

Conversation

DrJKL
Copy link
Contributor

@DrJKL DrJKL commented Sep 23, 2025 •
edited
Loading

No I am not proud of the new placeholder arguments.

Summary

Small change to unify the two composables before updating the logic.
Edit: Now unifies them both into the void.

┆Issue is synchronized with this Notion page by Unito

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 23, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 23, 2025 •
edited
Loading

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/24/2025, 04:16:01 AM UTC

📈 Summary

  • Total Tests: 460
  • Passed: 430 ✅
  • Failed: 0
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • ✅ chromium: View Report • ✅ 423 / ❌ 0 / ⚠️ 1 / ⏭️ 29
  • ✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • ✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • ✅ mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

jtydhr88
jtydhr88 previously approved these changes Sep 23, 2025
View reviewed changes
Copy link
Collaborator

@jtydhr88 jtydhr88 left a comment

Choose a reason for hiding this comment

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

LGTM

@DrJKL DrJKL enabled auto-merge (squash) September 23, 2025 23:17
Myestery
Myestery previously approved these changes Sep 23, 2025
View reviewed changes
Copy link
Collaborator

@Myestery Myestery left a comment

Choose a reason for hiding this comment

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

Explicitly passing out undefined looks weird too
Can we use an object instead as parameter?

@DrJKL
Copy link
Contributor Author

DrJKL commented Sep 23, 2025

Explicitly passing out undefined looks weird too Can we use an object instead as parameter?

That's what I'd prefer, yeah. I'll be changing the logic in here quite a bit, if not obviating the composable. I'll try to clean up the construction if it's still necessary.

@christian-byrne
Copy link
Contributor

Does this failure indicate a change in the behavior https://19cb4803.comfyui-playwright-chromium.pages.dev/#?testId=0bebc894f304375862db-e6498858489872433645?

@DrJKL
Copy link
Contributor Author

DrJKL commented Sep 24, 2025

Does this failure indicate a change in the behavior https://19cb4803.comfyui-playwright-chromium.pages.dev/#?testId=0bebc894f304375862db-e6498858489872433645?

Very probably

@DrJKL DrJKL dismissed stale reviews from Myestery and jtydhr88 via c985591 September 24, 2025 02:56
@DrJKL
Copy link
Contributor Author

DrJKL commented Sep 24, 2025

Does this failure indicate a change in the behavior https://19cb4803.comfyui-playwright-chromium.pages.dev/#?testId=0bebc894f304375862db-e6498858489872433645?

Okay, let's see if that worked.

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 24, 2025
@DrJKL DrJKL changed the title cleanup: unify useCanvasTransformSync composables. cleanup: remove useCanvasTransformSync composables. Sep 24, 2025
@DrJKL
Copy link
Contributor Author

DrJKL commented Sep 24, 2025

Just going to combine this with the subsequent cleanup PR.

@DrJKL DrJKL requested review from Myestery and jtydhr88 September 24, 2025 03:50
Myestery
Myestery approved these changes Sep 24, 2025
View reviewed changes
Copy link
Collaborator

@Myestery Myestery left a comment

Choose a reason for hiding this comment

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

LGTM

DrJKL and others added 3 commits September 23, 2025 21:05
@DrJKL
cleanup: unify useCanvasTransformSync composables.
fda4382 
No I am not proud of the new placeholder arguments.
@DrJKL
fix: Get the canvas from the store externally for the users of the or…
2d03f9b 
…iginal composable.
@DrJKL
Cleanup: remove useCanvasTransformSync (#5748)
0e01421 
## Summary

Replace the bespoke composable with a vueuse one that does the same
thing but a little nicer.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5748-Cleanup-remove-useCanvasTransformSync-2786d73d365081c29c56f4133af9bf80)
by [Unito](https://www.unito.io)
@DrJKL DrJKL force-pushed the drjkl/transform-events branch from dc89557 to 0e01421 Compare September 24, 2025 04:06
@DrJKL DrJKL disabled auto-merge September 24, 2025 04:35
@DrJKL DrJKL enabled auto-merge (squash) September 24, 2025 04:36
@DrJKL DrJKL disabled auto-merge September 24, 2025 04:36
@DrJKL DrJKL merged commit 6449d26 into main Sep 24, 2025
21 checks passed
@DrJKL DrJKL deleted the drjkl/transform-events branch September 24, 2025 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myestery Myestery Myestery approved these changes

@christian-byrne christian-byrne Awaiting requested review from christian-byrne

@jtydhr88 jtydhr88 Awaiting requested review from jtydhr88 jtydhr88 is a code owner

Assignees

@jtydhr88 jtydhr88

@arjansingh arjansingh

@simula-r simula-r

@Myestery Myestery

@christian-byrne christian-byrne

Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DrJKL @christian-byrne @jtydhr88 @Myestery @arjansingh @simula-r