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

Add HtmlPrefix parameter to IDisplayManager<TModel> #11612

Closed
MikeAlhayek opened this issue Apr 28, 2022 · 10 comments · Fixed by #11775
Closed

Add HtmlPrefix parameter to IDisplayManager<TModel> #11612

MikeAlhayek opened this issue Apr 28, 2022 · 10 comments · Fixed by #11775
Milestone

Comments

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Apr 28, 2022

Is your feature request related to a problem? Please describe.

I have a need to create a function that would create users in bulk from excel file. Now, that I have a collection of data (user data), I need a way to tell drivers the prefix that should be use when building the ShapeResult. For example, the UserDisplayDriver tries to bind the data from the request to EditUserViewModel. However, if the request has data that are prefixed with Users[0].User_ the binding engine would not know how to bind the data in the request to the viewmodel.

Describe the solution you'd like

Change the IDisplayManager to the following

    public interface IDisplayManager<TModel>
    {
        Task<IShape> BuildDisplayAsync(TModel model, IUpdateModel updater, string displayType = "", string groupId = "", string htmlPrefix = "");
        Task<IShape> BuildEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");
        Task<IShape> UpdateEditorAsync(TModel model, IUpdateModel updater, bool isNew, string groupId = "", string htmlPrefix = "");
    }

Now with that change, during the post request on creating the user I can do something like this

for(int i = 0; i < model.Record.Length; i++)
{
    var user = new User();
    var prefix = $"{nameof(model.Record)}[{i}]"; // prefix would be "Record[0]"
	
    var shape = await _userDisplayManager.UpdateEditorAsync(user, updater: _updateModelAccessor.ModelUpdater, isNew: true, htmlPrefix = prefix);

    await _userService.CreateUserAsync(user, null, ModelState.AddModelError);
}

If this is an acceptable approach I can submit a PR

@ns8482e
Copy link
Contributor

ns8482e commented Apr 28, 2022

IMO - Prefix is what driver author decides to keep to differentiate different inputs - You may want to dynamically build prefix inside driver if your driver needs to have specific prefix.

You may need implement your driver similar to FlowPart driver

@MikeAlhayek
Copy link
Member Author

I agreed that the driver should be defining the prefix. Adding prefix from the display manager would only concatenate to that prefix. so in the above example, the Prefix would be Record[0]. then the driver's own prefix. This would only be added if one explicitly added it.

@ns8482e
Copy link
Contributor

ns8482e commented Apr 28, 2022

As DisplayManager creates root shape and it should start with empty prefix.

You can always change prefix by defining your own driver

@MikeAlhayek
Copy link
Member Author

As DisplayManager creates root shape and it should start with empty prefix.

You can always change prefix by defining your own driver

In the above example, why would I have to define a new display driver just because the Prefix value would be different? The logic in the driver relay won't change. IMO, adding the ability to specify prefix won't change the default behavior but would give the consumer the flexibility to control the behavior when needed.

@ns8482e
Copy link
Contributor

ns8482e commented May 2, 2022

IMO if prefix is needed by parent shape for it’s child shapes then it should not be child shape’s display manager’s responsibility

@sebastienros
Copy link
Member

Same as @ns8482e, if FlowPart and BagPart can already do it, why not create a custom driver that defines the prefix for its inner content?

But if the change is simple and not risky (just an extensibility) then please provide a PR

@sebastienros sebastienros added this to the 1.x milestone May 5, 2022
@MikeAlhayek
Copy link
Member Author

Same as @ns8482e, if FlowPart and BagPart can already do it, why not create a custom driver that defines the prefix for its inner content?

But if the change is simple and not risky (just an extensibility) then please provide a PR

I think the idea here is not having to create drivers with the same logic as existing once just because the prefix is not the same. If you think about the use case I listed above, the logic to create a user won't really change. The only thing that changes here is where the request is coming from. So I read the user data from excel file, then render a the data in a grid view. When the user submit's the data, the logic in the UpdateAsync of existing of the drivers won't change but the binding prefix will. I just think this would make it easier for one to reuse drivers only by controlling the prefix. It won't change anything that currently exists, it just extends existing behavior to allow for covering more use-case. I'll try to sent a PR request

@ns8482e
Copy link
Contributor

ns8482e commented May 9, 2022

@CrestApps it’s not question about just extending behavior It’s like encapsulation, you don’t want to expose the prefix outside of root shape created by display manager

if you need for some reason you can always achieve such behavior by creating and registering display driver responsible for it

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented May 23, 2022

@ns8482e we have string htmlFieldPrefix = "" in IContentItemDisplayManager any reason why it's acceptable there but not acceptable on IDisplayManager<TModel>. These two manager do the same exact work except that IContentItemDisplayManager is concrete to ContentItem.

@ns8482e
Copy link
Contributor

ns8482e commented May 23, 2022

As I said earlier - it’s my opinion

even I don’t like to have html prefix as param in IContentItemDisplayManager as it adds inconsistency between display and edit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants