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

[Samples] from dynamic bindings to static VM ("SubModelOpt" sample) #607

Closed
wants to merge 2 commits into from

Conversation

YkTru
Copy link

@YkTru YkTru commented Jun 6, 2024

  • I propose to convert all actual samples using dynamic bindings to static bindings, or make 2 folders and put appropriate samples in each: "Dynamic bindings", "Static bindings"

  • I've run this new sample and everything works as expected, but I don't have enough knowledge to know if I've overlooked anything (e.g., not using the right helpers for better performance, missing relevant “>> Binding.x” (e.g., boxT, addLazy, etc.).

  • I've converted 2-space indentation to 4-space idiomatic indentation (in fact, why do all samples use 2-space indentation? it's not practical to refactor everything to 4-space if you need to explore/reuse/etc. the sample).

  • I suggest using (not idiomatic I know) “_Binding” for the binding helpers and “_ViewModel” for the static VM to improve readability and intellisense experience (i.e. writing “_V” filters automatically to get all static VMs; I do the same in WPF for "_Behaviors", "_Converter", "_ResourceDict" etc. which helps a lot IMO).

  • I suggest skipping 2 lines between the module and the associated ViewModel, and 3 between the previous ViewModel and a new module; I suppose this too is not idiomatic, but IME it helps a lot with readability/navigation.

  • I can do this kind of conversion from dyamic to static VM for all samples if you think it's relevant, it helps me learn at the same time. (I also intend to provide samples for what I think a “basic abstract project structure” that can scale well might look like (mostly based on previous discussions) if you think it's relevant too (?); this could at least be a starting point for possible samples of this kind, since I'm not an expert. Would another folder in the sample repo named “Project Structures” be relevant then?)


  • should:
let form2Visible_Binding =
    Binding.OneWayT.id
    >> Binding.mapModel (fun (m: App.Model) ->
        match m.Dialog with
        | Some (App.Dialog.Form2 _) -> true
        | _ -> false)

be replaced by:

    let form2Visible_Binding =
        Binding.oneWay (fun (m: App.Model) ->
            match m.Dialog with
            | Some (App.Dialog.Form2 _) -> true
            | _ -> false)

and same for all other binding helpers?


  • Problem: DataContext="{Binding Form1}" (and Form2) is the only binding unable to resolve at compile time (though at runtime it works fine):
<local:Form1
    d:DataContext="{Binding DataContext.Form1, RelativeSource={RelativeSource AncestorType=StackPanel}}"
    DataContext="{Binding Form1}"
    Visibility="{Binding DataContext.Form1Visible, RelativeSource={RelativeSource AncestorType=StackPanel}, Converter={StaticResource VisibilityConverter}}" />

codelens says: (Form1_ViewModel).Path=, is there a way to access (App_ViewModel).Path= here?

@YkTru
Copy link
Author

YkTru commented Jun 12, 2024

@marner2 ?

@YkTru
Copy link
Author

YkTru commented Jun 19, 2024

@TysonMN ? (sorry to tag you but I don't know who to ask else)

@marner2
Copy link
Collaborator

marner2 commented Jun 19, 2024

I think you're on to something here, but we don't want to replace the old versions yet. This all looks like a correct transform though at my first go through. I'll have to download the source to see what's going on with that binding issue. My guess is that it's something to do with that d:DataContext being different from the DataContext.

@TysonMN
Copy link
Member

TysonMN commented Jun 20, 2024

Sounds like you are doing too much in one PR. If you want to change from 2 to 4 spaces for intention, then do that in a PR of its own.

If you want people to review your work, then make your work easy to review.

@YkTru
Copy link
Author

YkTru commented Jun 26, 2024

@marner2 Great, when you say :

I think you're on to something here

1-Which part exactly do you think should be kept/revised/removed?

--

My guess is that it's something to do with that d:DataContext being different from the DataContext.

2-What do you mean "being different" since the code is:

d:DataContext="{Binding DataContext.Form1, RelativeSource={RelativeSource AncestorType=StackPanel}}"
    DataContext="{Binding Form1}"

?

Thank you

@YkTru
Copy link
Author

YkTru commented Jun 26, 2024

Sounds like you are doing too much in one PR. If you want to change from 2 to 4 spaces for intention, then do that in a PR of its own.

If you want people to review your work, then make your work easy to review.

@TysonMN You're absolutely right, changing the "spaces" add too many changes, would you advise me to make another PR with only the modified code, but keeping the 2-spaces indentation of the original sample? (sorry, I'm new to PR-ing)

@TysonMN
Copy link
Member

TysonMN commented Jun 26, 2024

Yes

@YkTru YkTru closed this by deleting the head repository Jun 26, 2024
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.

3 participants