Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Rework #42

Open
wants to merge 2 commits into
base: indev
Choose a base branch
from
Open

Rework #42

wants to merge 2 commits into from

Conversation

Deudly
Copy link
Member

@Deudly Deudly commented Jun 5, 2017

This Pull Request includes a lot of content

  • Changed a lot of code (Yes, that's why this is a rework)
  • Champion Select: Test Pick Mode (Like Blind Pick but enemy players can see your pick ^^)
  • Reconnect to Game screen (with Leave option)
  • New cool desing
  • Game starting works well now
  • Summoners Icons!
  • Removed Config File
  • Updated Socket.IO version

To-Do:

  • Update all dependencies
  • Bring back the old "choose lobby options" menu (with stuff like banned things, map select, champion select) so that the new Champion Select: Test Pick Mode is optional

@MythicManiac
Copy link
Member

MythicManiac commented Jun 6, 2017

Off the top of my head, I find the whole champion select phase enforcement unnecessary. The idea originally was (and still is) that it's an optional addition and that you don't have to use it by default unless you want to.

This change enforces the champion select phase, and removes the custom game lobby like feel that initially existed, which personally I'm not a fan of. I'm afraid if this is merged in it's current state, someone has to go back the git history regardless and dig up old pieces of code to make the champion select optional.

The skin itself looks great, though it'd be nice to make sure the custom setting components still work in the new skin as well.

As for the code review, this is a massive change (not to mention it's in a single commit) so it's a tad difficult to review. I'll do it some evening with a 🍺 on my hand when I don't mind the time spent. If you want to make sure this goes through before then, you should

  1. Make the champion select phase optional
  2. Make sure the different settings components still work in the new design

@Deudly
Copy link
Member Author

Deudly commented Jun 6, 2017

I won't continue this. CLOSING PR

@Deudly Deudly closed this Jun 6, 2017
@MythicManiac MythicManiac removed the ready label Jun 6, 2017
@Deudly Deudly reopened this Jun 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants