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

hugo/feature/Add action to DnDGrid in NewGEK #1649

Merged
merged 8 commits into from
Dec 17, 2024

Conversation

HPezz
Copy link
Contributor

@HPezz HPezz commented Dec 13, 2024

  • ✨ (NewGEK): Add DnDGridUIModel
  • 🧑‍💻 (NewGEK): Add new image choices set
  • ♻️ (NewGEK): GridCoordinator use GridUIModel
  • ♻️ (NewGEK): GridVM use coordinator's uiModel
  • ♻️ (NewGEK): DnDGrid now can display or not action
  • ♻️ (NewGEK): Display all sorts of actionThenDnDGrid

@HPezz HPezz requested a review from ladislas December 13, 2024 13:08
@HPezz HPezz self-assigned this Dec 13, 2024
@HPezz HPezz force-pushed the hugo/feature/Add-ActionThenDnD-to-NewGEK branch from ffd1cc4 to 75df390 Compare December 13, 2024 13:16
@HPezz HPezz changed the title hugo/feature/Add ActionThenDnD to NewGEK hugo/feature/Add action to DnDGrid in NewGEK Dec 13, 2024
Copy link
Member

@ladislas ladislas left a comment

Choose a reason for hiding this comment

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

that's some great work! 👍 I've added some feedback/requests on naming/architecture

@@ -31,6 +60,16 @@ public struct DnDGridView: View {
@StateObject private var viewModel: DnDGridViewModel
@State private var scene: SKScene = .init()

private var standardDnDGridView: some View {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can omit the standard and just have dndGridView or even gridView

I'm not sure if it's possible but for first class citizens like this, I would prefer to see (DnD)GridView as a struct with the needed parameters.

this allows for code reuse and when reading the code it makes it clear that this part is important and not just a local variable: this is where everything happens

Copy link
Member

Choose a reason for hiding this comment

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

the scene logic can be moved to a specific class

Copy link
Member

Choose a reason for hiding this comment

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

with that said, the current name DnDGridView for the file it a bit to narrow now that we added the action part. it's not just about the grid anymore.

maybe DnDExerciseViewGrid might be more suited as it's a full exercise now.

we would then have DnDExerciseViewGridWithZones, DnDExerciseViewOneToOne, etc.

@ladislas ladislas force-pushed the hugo/feature/Add-ActionThenDnD-to-NewGEK branch from f8c01d3 to 0bd2e74 Compare December 17, 2024 12:30
@ladislas ladislas merged commit 9c164ab into develop Dec 17, 2024
@ladislas ladislas deleted the hugo/feature/Add-ActionThenDnD-to-NewGEK branch December 17, 2024 12:30
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