LeagueSandbox / LobbyClient

League Sandbox's Launcher Client
17 stars 24 forks source link

Rework #42

Open Deudly opened 7 years ago

Deudly commented 7 years ago

This Pull Request includes a lot of content

To-Do:

MythicManiac commented 7 years ago

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 commented 7 years ago

I won't continue this. CLOSING PR