Skip to content
This repository has been archived by the owner. It is now read-only.

Add partial helper to Razor Page \ PageModel #7994

Closed
wants to merge 4 commits into from

Conversation

@hishamco
Copy link
Contributor

@hishamco hishamco commented Jul 1, 2018

This fixes issue #7885

@hishamco hishamco force-pushed the hishamco:partial-helper branch 3 times, most recently from 17b6188 to 5d8e457 Jul 1, 2018
@hishamco
Copy link
Contributor Author

@hishamco hishamco commented Jul 1, 2018

@pranavkm I have two questions:

1- Do we need to pass a ViewData for any reason as its suggested in the issue?
2- I disabled the ViewDate check because it make the tests fail, if i'm not wrong because the model is change by the helper. I'm not sure if I did something wrong or do you recommend something else?

@natemcmaster natemcmaster changed the base branch from dev to master Jul 2, 2018
/// <param name="model">The model to be passed into the partial.</param>
/// <returns>The created <see cref="PartialViewResult"/> object for the response.</returns>
public virtual PartialViewResult Partial(string partialViewName, object model)
=> new PartialViewResult

This comment has been minimized.

@pranavkm

pranavkm Jul 2, 2018
Member

Could you use a regular function here? The rule of thumb we've been using is to use lambdas for one-lines, regular functions otherwise.

This comment has been minimized.

@hishamco

hishamco Jul 3, 2018
Author Contributor

It's one line!!

/// </summary>
/// <param name="partialViewName">The partial name.</param>
/// <returns>The created <see cref="PartialViewResult"/> object for the response.</returns>
public virtual PartialViewResult Partial(string partialViewName)

This comment has been minimized.

@pranavkm

pranavkm Jul 2, 2018
Member

This should be called PartialView to keep inline with Controller: https://github.com/aspnet/Mvc/blob/master/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Controller.cs#L174. Also partialViewName -> viewName to keep the signatures similar

This comment has been minimized.

@hishamco

hishamco Jul 3, 2018
Author Contributor

Sure

=> new PartialViewResult
{
ViewName = partialViewName,
ViewData = new ViewDataDictionary(ViewData) { Model = model }

This comment has been minimized.

@pranavkm

pranavkm Jul 2, 2018
Member

If you follow the same pattern as controllers, you could get away without the need to new-ing this up: https://github.com/aspnet/Mvc/blob/master/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Controller.cs#L148-L155

This comment has been minimized.

@hishamco

hishamco Jul 3, 2018
Author Contributor

I already did that before, but I'm the test fails in the CI, coz I'm unable to test it in my end because VS gives me an error when I run the tests .. anyhow I will try to follow the pattern

@hishamco hishamco force-pushed the hishamco:partial-helper branch from 79771ee to ec63187 Jul 3, 2018
Copy link
Member

@pranavkm pranavkm left a comment

Could you add a set of functional tests for this

  1. Where a model is passed to the partial and the partial has an @model directive
  2. Where no model is passed and the model does not have an @model directive.

Just to make sure the feature works end-to-end.

@@ -1903,8 +1949,8 @@ public void ViewComponent_WithName()
{
PageContext = new PageContext
{
ViewData = viewData,

This comment has been minimized.

This comment has been minimized.

@hishamco

hishamco Jul 5, 2018
Author Contributor

You need me to leave the extra comma in the ViewComponent test?!!

This comment has been minimized.

@pranavkm

pranavkm Jul 5, 2018
Member

Yes please 👍

This comment has been minimized.

@hishamco

hishamco Jul 5, 2018
Author Contributor

Sure

This comment has been minimized.

@hishamco

hishamco Jul 5, 2018
Author Contributor

Done!! but i'm not sure what's the need for the extra comma!!

@pranavkm pranavkm changed the base branch from master to release/2.2 Jul 5, 2018
@hishamco
Copy link
Contributor Author

@hishamco hishamco commented Jul 5, 2018

I 'll try to do end to end functional tests, but the problem my VS doesn't run the tests and showed me an error in the analyzer!!

@pranavkm
Copy link
Member

@pranavkm pranavkm commented Jul 5, 2018

Let me know if you have trouble writing these, I can do a follow up.

@hishamco
Copy link
Contributor Author

@hishamco hishamco commented Jul 5, 2018

I will share with you the error message

@hishamco hishamco force-pushed the hishamco:partial-helper branch 11 times, most recently from 07b05cb to 6e4bd34 Jul 5, 2018
- release/*

resources:
repositories:
- repository: buildtools
type: git
name: aspnet-BuildTools
ref: refs/heads/release/2.2

This comment has been minimized.

@pranavkm

pranavkm Jul 6, 2018
Member

Looks like you rebased on the wrong branch. Could you fix this up?

This comment has been minimized.

@hishamco

hishamco Jul 7, 2018
Author Contributor

Oops .. I fix it

@hishamco hishamco force-pushed the hishamco:partial-helper branch from 6e4bd34 to 8cf6108 Jul 7, 2018
@hishamco hishamco force-pushed the hishamco:partial-helper branch from 8cf6108 to e511951 Jul 7, 2018
@hishamco
Copy link
Contributor Author

@hishamco hishamco commented Jul 7, 2018

@pranavkm seems everything as expected after I fixed the unit test issue in my VS, please let me know everything fine I will squash the commits

@pranavkm
Copy link
Member

@pranavkm pranavkm commented Jul 9, 2018

Merged in dee479f. Thanks for the PR!

@pranavkm pranavkm closed this Jul 9, 2018
@hishamco
Copy link
Contributor Author

@hishamco hishamco commented Jul 9, 2018

I think it's the time to update your sample in the entropy repo, I will push a PR soon

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants