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

fix: move scene ui fields to update cycle #5

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

chrisk-7777
Copy link
Contributor

@chrisk-7777 chrisk-7777 commented Jul 10, 2022

Resolves bug discussed in excaliburjs/Excalibur#2399

Line 705 of dev-tools is slightly heavy, but it needs to account for both dynamic scenes added and dynamic scenes added and activated. I've demonstrated this in the last example in index.ts. Even with a lot of scenes, I think this check would still be quick enough for development.

In the examples, pressing "N" will add a scene, but not activate it, and pressing "A" will both add and activate a scene. In both cases we expect to see the scene list in the dev tools ui to update.

Additional information

Side question - I had a bit of trouble using npm link to test my unpublished version against a local project. To be fair, I didn't dig too deep, but is that something that works on your end?

index.ts Show resolved Hide resolved
Copy link
Member

@eonarheim eonarheim left a comment

Choose a reason for hiding this comment

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

@chrisk-7777 Thanks again! This looks great, and is very appreciated!

@eonarheim
Copy link
Member

@chrisk-7777 Great questions!

It seems like this repo and the core repo follow more of a commitizen style of fix, feat, chore, etc, so I followed that here too

Excellent, that's definitely our preference. I think we moved to that style over time (I'll take some time with the other contributors to review and update the commit style and make sure the CONTRIBUTING.md is fully up to date)

first PR around these parts, feel free to go hard, good to get alignment at the start

For sure 👍 this looks exactly what I would have expected so I have no feedback this time

Side question - I had a bit of trouble using npm link to test my unpublished version against a local project. To be fair, I didn't dig too deep, but is that something that works on your end?

I've run into this before, parcel seems to heavily cache the npm dependencies so I've found I have to rebuild and manually clear the .parcel-cache/dist in various repos to make it work. There may a parcel configuration to help with this but I'm not sure what it is.

Also this repo is a little weird it uses parcel for local dev, and webpack to produce the final bundle npm run build so be sure you're also running that when linking to other repos 👍

@eonarheim eonarheim merged commit 47d172b into excaliburjs:main Jul 11, 2022
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