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

Re-added ACTWebSocket support #77

Merged
merged 7 commits into from
Jul 21, 2019

Conversation

Rawrington
Copy link
Contributor

@Rawrington Rawrington commented Jul 16, 2019

I have re-added ACTWebSocket support while maintaining OverlayPlugin compatibility, this build works with both ACTWS and OverlayPlugin from github pages, and does not have any underlying issues, I also fixed an issue wherein limit breaks and other strange things would cause a combatant to get cut off at the end of the list. Additionally I have updated package.json to work under the latest NPM with a bunch of updated dependancies.

@Rawrington
Copy link
Contributor Author

NOTE: This has been tested with both plugins on my end through github pages
(https://rawrington.github.io/horizoverlay/)

@Rawrington
Copy link
Contributor Author

(Currently doing a bug fix as I noticed an issue)

@Rawrington
Copy link
Contributor Author

testing it now, should be fixed though.

@bsides
Copy link
Owner

bsides commented Jul 17, 2019

Awesome, can't wait to test it too! Will give it a try tonight to push this ASAP.
Thank you so much for your efforts!!!

@bsides bsides self-requested a review July 17, 2019 17:14
Copy link
Owner

@bsides bsides left a comment

Choose a reason for hiding this comment

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

Ok, I don't think the code need any change, I'll just test it later to see if there's something else. Other than that great work 👍

package.json Outdated
},
"homepage": "https://bsides.github.io/horizoverlay",
"homepage": "https://rawrington.github.io/horizoverlay",
Copy link
Owner

Choose a reason for hiding this comment

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

The reference from the original repo should still be the same. Could you make this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I'll sort it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh it was sorted

">0.2%",
"not dead",
"not op_mini all"
]
Copy link
Owner

Choose a reason for hiding this comment

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

I believe we should check which version the Overlay plugin uses so we can deploy with that target in mind. I'll let you know in my tests tonight which version is it. It'd be better to other developers to not be confused with API changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah true, this specific combination of requirements DOES work with overlayplugin which is why i chose it

@@ -12,7 +12,12 @@ class Combatants extends Component {
render() {
const maxRows = this.props.config.maxCombatants
const dataArray = Object.keys(this.props.data)
const battler = dataArray.slice(0, maxRows)
const regexp = /[a-zA-Z]+\s+[a-zA-Z]+/g;
const battler = dataArray.filter(player => (
Copy link
Owner

Choose a reason for hiding this comment

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

Just one little thing here: do you have prettier installed in your IDE? If not, I'll set a thing later to make this better indented once we push our files (ie. a post-commit hook).

Copy link
Owner

Choose a reason for hiding this comment

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

...and you've done it here. Reviews should begin with the last ones, heh!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually fixed spacing in a later thing :P

package.json Outdated
},
"homepage": "https://rawrington.github.io/horizoverlay",
"homepage": "https://bsides.github.io/horizoverlay",
Copy link
Owner

Choose a reason for hiding this comment

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

Oh well forget about my last comment about this 😅

@Rawrington
Copy link
Contributor Author

I've changed how the slicing works in my personal build and cut out jobless/outputless party members to help with party filter settings in ACT, I'm thinking of implementing multiple rows so people can show the full 24 people if they want on 24 mans (i.e. a rows setting in config)

@Rawrington
Copy link
Contributor Author

I can push this to my be merged branch if you want me to :P

@Rawrington
Copy link
Contributor Author

my latest commit should sort out all the issues, i added in my 2 filters, you can change the show jobless combatants option in config to put the behaviour back to how it originally behaved :P

@bsides
Copy link
Owner

bsides commented Jul 18, 2019

I can push this to my be merged branch if you want me to :P

Looks nice, did you make it as an option? These days have been crazy to test it though 😭

@bsides
Copy link
Owner

bsides commented Jul 21, 2019

Although I cannot test it, I'll just accept this. If anything goes wrong we can fix or just throw back the older version.

@bsides bsides merged commit 6308c50 into bsides:master Jul 21, 2019
bsides added a commit that referenced this pull request Jul 21, 2019
bsides added a commit that referenced this pull request Jul 21, 2019
bsides added a commit that referenced this pull request Jul 21, 2019
bsides added a commit that referenced this pull request Jul 21, 2019
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