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 route values to Region MvcData and Controller Actions #29

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

willprice76
Copy link
Contributor

Update to allow Region MvcData to also have route Values (similar to Entity MvcData). This allows for the parameterisation of Region views (see issue #28). The implementation in the DefaultModelBuilder is to get the route values from the CT metadata (routeValues), as for entities. In future this can be updated to a more normalised method when Regions are properly modelled in the CM.

Now I can create a single generic N-Column region view and specify regionCols and regionType route values on the CT metadata (and different region names like 3-Column, 2-Column etc.), which enable me to configure the view column layout logic and render different class and other HTML attributes depending on the regionType.

… to configure region views (for example providing #cols and displaytype to generic region view for column layout)
@rpannekoek
Copy link
Contributor

We should deal with "conflicts": multiple CTs for the same Region (name), but with different route values; the first one will "win", but we should log the fact that there are conflicts (like we do for Region view name).
I think it would be better to distinguish Region route values from Entity route values; either by using a separate metadata field (my preference) or using prefixes in the route value keys. That will reduce the chance of such conflicts and enable us to detect them.
For example, I may want to use two Entities with different Entity route values in the same Region. They may have the same Region route values (if any), so don't conflict on that level, but currently we can't tell if there is a conflict or not.

@willprice76
Copy link
Contributor Author

Understood - I will update to log conflicts - Given that the CM implementation is temporary (a workaround until there are proper regions in the CM), why not reuse the existing routeValues metadata field and pick up anything with a prefix 'region' for now?

@rpannekoek
Copy link
Contributor

Isn't a separate metadata field just as easy, more explicit and hence clearer for a user?
You can specify two sets of key/value pairs: one for Region level and one for Entity Level. The one on Region level must be consistent for all Entities in the Region, the one on Entity level doesn't.
Indeed, this is a suboptimal, temporary solution anyways, but like you said let's do a best effort to make the best of it.
BTW: from a user-perspective I wouldn't call those things "route values" but "parameters" (use of "route values" for that purpose is a technical solution using ASP.NET MVC terminology). But on the other hand we are already using this terminology for Entity MVC data too (both in .NET and Java), so for consistency it makes sense to keep using that term, we could just let the field label/description be "parameters".

@willprice76
Copy link
Contributor Author

OK - you convinced me - I will make that update

@willprice76
Copy link
Contributor Author

Note the change in PR #27 - this means that rather than logging conflicts in region mvc data, we can simply start rendering a new duplicate region - which is the desired behaviour if we have, for example 2 consecutive 3-Column regions, but the first having different route values than the second.

@willprice76
Copy link
Contributor Author

Note that I have added the regionRouteValues metadata field after the regionName metadata field, and now have following Descriptions for the CT metadata fields:

  • routeValues: Entity Parameters (comma separated list of name:value pairs)
  • regionRouteValues: Region Parameters (comma separated list of name:value pairs)

@rpannekoek
Copy link
Contributor

TSI-1944 (internal issue ID for tracking purposes)

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.

None yet

2 participants