-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fixed Order Flipping (and sorted some tab issues) #82
base: master
Are you sure you want to change the base?
Conversation
…potential bugs from cropping up
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.
Just removing the comments would do!
Thanks again for your amazing time to help! I'm going to find time soon to rewrite into a v2 with a more detailed view of helper functions that nowadays are everywhere.
let dataArray = Object.keys(this.props.Combatant) | ||
let battler = dataArray.slice(0, maxRows) | ||
let combatant | ||
const maxRows = this.props.config.maxCombatants //not sure why let was being used here |
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.
Probably because I changed it after, I was learning es6 at this time and didn't have a linter on - good catch though :)
combatant.name = this.props.config.characterName | ||
//NOTE: instead of wasting time slicing the array up we just break the loop when we need to, it avoids the problem of filtering out the lb only to want to handle it later | ||
for (const battler of dataArray) { | ||
const combatant = this.props.Combatant[battler] //scope means we can just use const here |
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 thank you for explaining your steps, I know it may look weird in some places but I never refactored since v1 😉
Could you then remove the comments? They won't make sense after the PR is accepted.
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 will remove the comments I'm busy with savage raids and life at the moment but I'll get round to it as soon as possible
@@ -40,8 +36,16 @@ class OverlayRaw extends React.Component { | |||
10 | |||
) | |||
) | |||
break | |||
continue //break will just break the loop entirely.. if someone is deaing less damage than LB (yikes) they wouldn't end up included |
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.
Here too if you can
@@ -28,6 +28,7 @@ export default function initActWebSocket() { | |||
) | |||
} | |||
} | |||
return true | |||
} | |||
|
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.
Why are we returning boolean?
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.
we need to identify if ACTWebSocket is in use to flip the order in a couple places, we don't want to perform regex (easiest way to check that is old chromium compatible) everytime a new render is requested and gone throguh the DPS so we pass it through here as a prop to components that need the information
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 think I didn't understand 😢
I'm asking why did you change this specific function to return a boolean instead of void (no value or undefined). I understood you need to check if ACTWebSocket is in use but from what I could see it's not from the value of this function, right? Or is it?
This Fixes #81 and cleans up some tabs, the commits are a bit wonky cause I'm not the greatest with GIT but it should work the diff looks fine.