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

Simplify listGlobalSequences to hasGlobalSequences. #14

Merged
merged 1 commit into from
Dec 31, 2023

Conversation

Mat2095
Copy link
Contributor

@Mat2095 Mat2095 commented Dec 31, 2023

  • since global sequences are now played all together and can't be played individually, listing them is not very useful, so I replaced listGlobalSequences with hasGlobalSequences to check whether global sequences are present
  • renamed onGlobalSequenceChange to onPlayGlobalSequencesChange to reflect the new variable-name

Previously this PR hat more changes, but it was decided to split them into two PRs:

  • now you can select different variations for sequences from the drop-down
  • renamed params.sequences to params.sequence since it refers to a single sequence and not multiple ones

@Mugen87
Copy link
Owner

Mugen87 commented Dec 31, 2023

Do you mind if we split up the PR into two 😇 ? I would like to merge point 1 and 2 first.

The third one maybe requires more thought. I'm not sure yet how to manage variations, tbh.

What do you think about making the variations handling more explicit and add a variations drop down if a sequence has variations and maybe add listVariations( id ) on manager level?

@Mat2095
Copy link
Contributor Author

Mat2095 commented Dec 31, 2023

Do you mind if we split up the PR into two 😇 ? I would like to merge point 1 and 2 first.

Sure, we can do that. What do you mean by point 1 and 2 in that case?

The third one maybe requires more thought. I'm not sure yet how to manage variations, tbh.

What do you think about making the variations handling more explicit and add a variations drop down if a sequence has variations and maybe add listVariations( id ) on manager level?

I thought about that as well. Other viewers switch between variations randomly, but that seems more complicated and I kindof like the idea of being able to choose the variation manually. I had no preference between having having sequence and variation in one list or splitting it into two lists, so I chose the option that seemed simpler. But if you prefer having a separate drop-down, I will do that.

@Mugen87
Copy link
Owner

Mugen87 commented Dec 31, 2023

Sure, we can do that. What do you mean by point 1 and 2 in that case?

Replace listGlobalSequences() with hasGlobalSequences() and the rename onGlobalSequenceChange () to onPlayGlobalSequencesChange().

But if you prefer having a separate drop-down, I will do that.

Okay, then I would try this approach first and so how it works out.

@Mat2095 Mat2095 changed the title Show different variations in sequence-list. Simplify listGlobalSequences to hasGlobalSequences. Dec 31, 2023
@Mat2095
Copy link
Contributor Author

Mat2095 commented Dec 31, 2023

Okay, I've updated this PR to only refactor those two things.
I'll open another PR for the variations when I'm done.

@Mugen87 Mugen87 merged commit c5947bb into Mugen87:main Dec 31, 2023
2 checks passed
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