-
Notifications
You must be signed in to change notification settings - Fork 6
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
update to 0.8.6 #13
base: master
Are you sure you want to change the base?
update to 0.8.6 #13
Conversation
make the module work for 0.8.6 and not throw deprecation warnings. Ideally it is advised to rework some of the parts to work more natively with 0.8.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it only deprecation fixes? I haven't tested at all so I'm curious to know what exactly was broken. I am still going to test later today.
src/about-face.js
Outdated
const scene = token.object.scene; | ||
if (!AboutFace.sceneEnabled) return; | ||
token = (token instanceof Token) ? token : canvas.tokens.get(token.id); | ||
if (!token.children.includes(AboutFace.tokenIndicators[token.id].c)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we want to use that kind of coding standard, but I would move this condition into a named method so we can easily know what is being checked.
I don't know if it's because I am still new to module development, but I have no idea what is being checked here other than we check for a specific children.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I didn't want to use something like that as well, but for some reason it was removing the indicator from the children of the token, I wasn't able to understand why, but I really wanted the module to work, so I did this. It would be great if you could find a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're closer to the solution than I am. I won't be able to suggest a better solution than this without going as deep into the code as you did.
However to make life easier for the next person coming across this code, the condition !token.children.includes(AboutFace.tokenIndicators[token.id].c)
should be moved into a function or const
with a name similar to tokenChildrensIncludes###something###
and the use that const
inside the if
. I would do it, but I do not know what AboutFace.tokenIndicators[token.id].c
represents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to AboutFace.tokenIndicators[token.id].c
to a separate variable and added some comments.
Any news on this pull request? Seems like some much needed updates.. |
make the module work for 0.8.6 and not throw deprecation warnings. Ideally it is better to rework some of the parts to work more natively with 0.8.x