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

Renamed SummaryAdmin shapes of ContentsDriver #7086

Merged
merged 3 commits into from
Oct 1, 2020

Conversation

ns8482e
Copy link
Contributor

@ns8482e ns8482e commented Sep 16, 2020

Following ContentsDriver creates four shapes with different shape types.

Shape("Contents_SummaryAdmin__Tags", new ContentItemViewModel(model)).Location("SummaryAdmin", "Tags:10"),
Shape("Contents_SummaryAdmin__Meta", new ContentItemViewModel(model)).Location("SummaryAdmin", "Meta:20"),
Shape("Contents_SummaryAdmin__Button__Edit", new ContentItemViewModel(model)).Location("SummaryAdmin", "Actions:10"),
Shape("Contents_SummaryAdmin__Button__Actions", new ContentItemViewModel(model)).Location("SummaryAdmin", "ActionsMenu:10")

However the actual shape type is considered before __ i.e. all above shapes are resolved to the same shape type Contents_SummaryAdmin

Renamed these shapes as following to apply unique placement record for each shape

Contents_SummaryAdmin__Tags renamed to ContentsTags_SummaryAdmin
Contents_SummaryAdmin__Meta renamed to ContentsMeta_SummaryAdmin
Contents_SummaryAdmin__Button__Edit renamed to ContentsButtonEdit_SummaryAdmin
Contents_SummaryAdmin__Button__Actions renamed to ContentsButtonActions_SummaryAdmin

so that placement will target single shape type.

{
    "ContentsButtonActions_SummaryAdmin": [
      {
        "shape":"ContentsButtonEditNoView_SummaryAdmin"
      }
    ]    
}

Fixes #7002

@deanmarcussen
Copy link
Member

So the issue is that using a seperator in the shape name stops placement from hitting it?

My gut feel is that we might be better off renaming the shapes, than having a differentiator that noone will know about?

A differentiator would normally have something regarding a content item / part / field name, so it's a bit weird to be differentiating between zones.

Related #5151 (feel free to add a comment about how placement is impacted by seperators @ns8482e

@deanmarcussen
Copy link
Member

Or we could be adding alternates oob as is commented 10 lines above

var contentsMetadataShape = Shape("ContentsMetadata", new ContentItemViewModel(model)).Location("Detail", "Content:before");

@ns8482e
Copy link
Contributor Author

ns8482e commented Sep 21, 2020

So the issue is that using a seperator in the shape name stops placement from hitting it?

Yes, It's because original shape type is identified before __ in shape templates. Here! The original shape type for Contents_SummaryAdmin__Tags , Contents_SummaryAdmin__Meta, Contents_SummaryAdmin__Button__Edit, and Contents_SummaryAdmin__Button__Actions is Contents_SummaryAdmin hence placement is applied to all shapes

Related #5151 (feel free to add a comment about how placement is impacted by seperators @ns8482e

Yes, I can update the PR to change ShapeType to to not have _ or __ and change the template name without using differentiator.

e.g.

Contents_SummaryAdmin__Tags ----> SummaryAdminTags ---->SummaryAdminTags.cshtml
Contents_SummaryAdmin__Meta ----> SummaryAdminMeta ----> SummaryAdminMeta.cshtml
Contents_SummaryAdmin__Button__Edit ----> SummaryAdminButtonEdit ----> SummaryAdminButtonEdit.cshtml
Contents_SummaryAdmin__Button__Actions ----> SummaryAdminButtonActions ----> SummaryAdminButtonActions.cshtml

@deanmarcussen
Copy link
Member

Contents_SummaryAdmin__Tags ----> SummaryAdminTags ---->SummaryAdminTags.cshtml

Wouldn't it be Contents_SummaryAdmin__Tags ----> ContentsTagsSummaryAdmin ?

@ns8482e
Copy link
Contributor Author

ns8482e commented Sep 28, 2020

Wouldn't it be Contents_SummaryAdmin__Tags ----> ContentsTagsSummaryAdmin ?

Sure It will work just thought to remove prefix Contents to avoid confusion with other shapes as Here it's not representing stereo type. These shapes will be use for all content items regardless of their stereo type.

@deanmarcussen
Copy link
Member

Wouldn't it be Contents_SummaryAdmin__Tags ----> ContentsTagsSummaryAdmin ?

Sure It will work just thought to remove prefix Contents to avoid confusion with other shapes as Here it's not representing stereo type. These shapes will be use for all content items regardless of their stereo type.

I think the Contents was more an indicator which driver it was produced by, not related to stereotype.

I would keep it, otherwise there is no indicator that the shape is even content related.

It would be good to see the change, and I want to discuss this at triage

  • This could be achieved by a shape provider
  • Should placement work for everything?
    • Placement has been historically focused on front end display, but there are now more requests / expectations that it will work with all the admin shapes
    • If so, we have a larger shape renaming job to come, and a better shape name convention that we need to follow, that would need to reduce seperators when they have no specific meaning (like in this case, the seperator has no meaning, it is just getting in they way)

@ns8482e
Copy link
Contributor Author

ns8482e commented Sep 30, 2020

  • we have a larger shape renaming job to come, and a better shape name convention that we need to follow

Yes It's related #5151 and #5126

I think the Contents was more an indicator which driver it was produced by, not related to stereotype.

Ok, I agree for Contents prefix, if it's indicator of the driver - Then I feel SummaryAdmin suffix is redundant, and the shape type should be without displayType suffix i.e.. ContentsTags

And it's also align with the other shape ContentsMetadata produce by same driver but for Detail display type.

So following placement would more appropriate

"ContentsTags": [
      {
        "displayType" : "SummaryAdmin",
        "place":"-"
      }
    ]

compared to

"ContentsTagsSummaryAdmin": [
      {
        "displayType" : "SummaryAdmin",
        "place":"-"
      }
    ]

@deanmarcussen
Copy link
Member

ContentsTags_SummaryAdmin

@ns8482e
Copy link
Contributor Author

ns8482e commented Sep 30, 2020

ContentsTags_SummaryAdmin

Read display type from context or hardcode in shape type? e.g.

Shape("ContentsTags_SummaryAdmin", new ContentItemViewModel(model)).Location("SummaryAdmin", "Tags:10"),
or
Shape($"ContentsTags_{context.DisplayType}", new ContentItemViewModel(model)).Location("SummaryAdmin", "Tags:10"),

@ns8482e ns8482e changed the title Add Differentiator for Contents_SummaryAdmin shapes Renamed SummaryAdmin shapes of ContentsDriver Sep 30, 2020
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.

4 participants