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

2.0: refactor changing aliases in state and fix loading bar bug #2704

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

six7
Copy link
Collaborator

@six7 six7 commented Apr 28, 2024

Why does this PR exist?

Closes #2642

There was a loading bar that appeared when you renamed a token group, that never disappeared.

What does this pull request do?

The change fixing this issue is actually fairly small, I just removed the job that we had for this. The job itself was not an async operation, so there was no need for the loading bar. I assume it could have been a race condition that caused the start job and the stop job to never be submitted.

However, doing this I refactored how we call that updateAlias function as I discovered we had quite a mess there including a lot of logic directly in state which was not tested, a updateOtherAlias function which had no use. It basically was required as previously we didnt update modifier props when we update the aliases, but now I changed that to just happen before we perform the alias change, and added tests for that function, plus moved it to a dedicated new utils directory in the store where we should move all logic that currently clutters the tokenState.

Testing this change

Test coverage is pretty good with that one, but here's a video demonstrating the change:

CleanShot.2024-04-28.at.13.38.00.mp4

Additional Notes (if any)

We should really start to clean up our tokenState and move all the logic into dedicated files that we can test in isolation.

Copy link

changeset-bot bot commented Apr 28, 2024

⚠️ No Changeset found

Latest commit: f942343

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@six7 six7 marked this pull request as ready for review April 28, 2024 11:50
@six7 six7 mentioned this pull request Apr 28, 2024
@six7 six7 merged commit f168786 into release-139 Apr 30, 2024
5 of 6 checks passed
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.0]: Renaming a token group causes plugin to get stuck in Hold on, updating
2 participants