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

Remove wrapDispatch and add mapMsgWithModel #256

Merged
merged 12 commits into from
Aug 10, 2020
Merged

Conversation

TysonMN
Copy link
Member

@TysonMN TysonMN commented Aug 9, 2020

This PR is a merge into the new branch v4. It replaces the draft PR #244.

@cmeeren, there are conflicts between this branch and your logging branch. Do you mind if we review and complete this PR first and then you resolve the conflicts with your logging branch?

@cmeeren

This comment has been minimized.

@cmeeren
Copy link
Member

cmeeren commented Aug 9, 2020

Is there anything outstanding here? Anything I should review that I haven't already?

@cmeeren
Copy link
Member

cmeeren commented Aug 9, 2020

Come to think of it, IMHO the PRs should include relevant documentation/README updates, such as changelog items, migration steps, and relevant changes to the tutorial.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 9, 2020

I updated the release notes. What other documentation changes would you like ot see?

@TysonMN TysonMN mentioned this pull request Aug 9, 2020
9 tasks
@cmeeren
Copy link
Member

cmeeren commented Aug 9, 2020

Please remove the "Wrapping dispatch" section of the tutorial.

I'd also like the tutorial to explain and demonstrate the new functions. Where/when are they useful, how to use them correctly, any gotchas to watch out for if applicable, etc. In short, the simplest possible explanation (but no simpler) that will make users who have no idea they need these functions understand how great they are and how/when they can use them and how it improves their code. :)

@TysonMN
Copy link
Member Author

TysonMN commented Aug 10, 2020

I updated the tutorial. How does that look?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 10, 2020

Is there anything outstanding here?

Another thing to review is all the types I exposed in this commit. Should I add ///-documentation to all these types?

@cmeeren
Copy link
Member

cmeeren commented Aug 10, 2020

I updated the tutorial. How does that look?

Looks good! Could you also add something about how it enables you to factor out XAML controls? That's a (perhaps the?) major benefit, as I understand it.

Another thing to review is all the types I exposed in this commit. Should I add ///-documentation to all these types?

Could you make the implementations internal? type X = internal { ... }. I see no reason to make the internals of those types public.

Actually, why are they public in the first place?

@TysonMN
Copy link
Member Author

TysonMN commented Aug 10, 2020

Could you also add something about how it enables you to factor out XAML controls? That's a (perhaps the?) major benefit, as I understand it.

The XAML can already be extracted. For the SubModelSeq sample, I extracted it about six weeks ago in this commit that is already in master. The point related to the XAML is that it is now simpler and easier to create a design-time view model for the extracted XAML. For the SubModelSeq sample, I did that in this commit in the of this PR.

I will add something about this to the tutorial.

Actually, why are they public in the first place?

Ah, yes! You are right. Thanks for pushing back on this. I wasn't being very precise when I did that commit. Conceptually what I wanted was to publicly expose the six mapping functions (three in Bindings module and three in the Binding module). I carelessly did that by only removing internal from the two modules, which was necessary, but the Binding module also contained mapData and subModelSelectedItemLast. To publicly expose those two functions requires making more things public.

Instead, I just rebased into my branch this commit that publicly exposes the six mapping functions but keeps everything else internal.

So I will add ///-style documentation comments to those six functions.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 10, 2020

As you have noticed (but repeating here for the record), I have added to the tutorial about the design-time benefit that comes from the mapping functions and added ///-style documentation (and good type annotations and explicitly accepted all arguments with good names) to the mapping functions.

@cmeeren
Copy link
Member

cmeeren commented Aug 10, 2020

Looks good to me. Feel free to merge!

@TysonMN
Copy link
Member Author

TysonMN commented Aug 10, 2020

Looks good to me. Feel free to merge!

Great! :D

I squashed some commits. I will complete after the automated build finishes successfully.

@TysonMN TysonMN merged commit 6d4ac57 into elmish:v4 Aug 10, 2020
@TysonMN TysonMN deleted the mapMsgWithModel branch August 10, 2020 19:09
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