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

Rebase to fix conflicts #190

Closed

Conversation

JonnyVR1
Copy link

No description provided.

@JonnyVR1 JonnyVR1 force-pushed the bh-Added-difficulty-filters branch from ce46389 to 9864566 Compare March 20, 2022 21:03
@BlazingTwist
Copy link
Contributor

The Fix CustomJSONData path commit doesn't really belong in this repository.
You should check the Readme on how to properly resolve that.

Also, that looks like a lot more changes than just implementing the difficulty filters.
It would probably be easier to just re-create the changes based on the current master branch.

That being said, I think adding 5 extra buttons to the filters just for difficulty introduces a lot of clutter.
It might be nice to consolidate the Ranked/Unranked, Played/Unplayed and Easy/Normal/Hard/E/E+ buttons into dropdowns.

@JonnyVR1
Copy link
Author

The Fix CustomJSONData path commit doesn't really belong in this repository. You should check the Readme on how to properly resolve that.

Errr what? This commit uses the standard that is in the rest of the file - i.e. $(BeatSaberDir) rather than direct drive paths.

@BlazingTwist
Copy link
Contributor

Oh wow, I somehow read that the wrong way around. Thought you replaced the placeholder with an absolute path.

@halsafar
Copy link
Owner

Sorry for the delay on this. Looks good from a light review perspective. Going to do some testing tonight and a more thorough review.

@halsafar halsafar mentioned this pull request Mar 25, 2022
@halsafar
Copy link
Owner

So while I like this changeset it is as @BlazingTwist says there is a lot more here than necessary to implement difficulty filters.

  • The CustomJsonData path fix should be separate as there is no question we want that. I might snag that change asap.
  • The difficulty filter is good, but needs testing, should be separate.
  • The refactor of using var everywhere is unnecessary to implement the above two. We can debate the merit of var vs explicit typing (I personally prefer explicit typing) and I'm happy to accept changes but it should be separate from a feature implementation.

@halsafar halsafar closed this Mar 26, 2022
@JonnyVR1 JonnyVR1 deleted the bh-Added-difficulty-filters branch October 15, 2022 21:14
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.

4 participants