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

Inconsistent usage of "Display Type" terminology #17366

Open
mroskamp opened this issue Jan 16, 2025 · 6 comments · May be fixed by #17389
Open

Inconsistent usage of "Display Type" terminology #17366

mroskamp opened this issue Jan 16, 2025 · 6 comments · May be fixed by #17389
Labels
Milestone

Comments

@mroskamp
Copy link
Contributor

mroskamp commented Jan 16, 2025

Describe the bug

Typically, "Display Type" refers to the render context (e.g. Detail, Summary, Admin, SummaryAdmin). See:

However, the Placement documentation uses "Display Type" to mean, seemingly, two different things:

  1. It refers to the typical render contexts (Detail, Summary, etc.):
    - `displayType` (Optional): The display type, like `Summary` and `Detail` for the most common ones.
  2. It also uses "Display Type" in reference to the "Display" and "Edit" contexts: https://github.com/OrchardCMS/OrchardCore/blob/1814599ecfce9f41d7eca459cea8887f160c0dff/src/docs/reference/modules/Placement/README.md?plain=1#L101C31-L101C43

Orchard Core version

commit c5ed897 / #17333

To Reproduce

N/A

Expected behavior

Are both of these really "Display Types" or should the Display/Edit use different terminology?

Logs and screenshots

Image
Image

@MikeAlhayek
Copy link
Member

displayType From memory, placement refers to the same display type as in the

If we have a shape type Test, to hide it from the edit screen we do this

{
    "Test_Edit": {
        "place": "-"
    }
}

To hide it from being displayed on Summary display type:

{
    "Test": {
        "displayType": "Summary",
        "place": "-"
    }
}

Did I misunderstand your request or is my assessment is invalid?

@mroskamp
Copy link
Contributor Author

@MikeAlhayek, I agree with the examples you provided. The typical display types are Detail, Summary, etc.

My confusion comes from this part of the documentation introduce in commit c5ed897:

Specifically, in this line:

1. The Shape must include the display type. For example `TextField_Display`.

This might further highlight the issue:
Image

The "DisplayType" in [ContentPart]-[ContentField]_[DisplayType]__[DisplayMode] seems to refer to something different than what we typically refer to as "display types" (e..g Detail, Summary, etc.).

Instead, it seems to the literal _Display__ "display separator" seen here:

Or the literal _Edit__ separator built up here:

return GetEditorShapeType(typeof(TField).Name + "_Edit", partFieldDefinition);

I think this is mostly just a case of using the wrong terminology. The _Display__ and _Edit__ separators aren't display types, are they? If not, is there a better general term that applies to those separators?

Or should we just replace:
[ContentPart]-[ContentField]_[DisplayType]__[DisplayMode]

with:
[ContentPart]-[ContentField]_Display__[DisplayMode]
(this ignores the _Edit__ case, but seems to be consistent with elsewhere in the documentation, see here and here)

@MikeAlhayek
Copy link
Member

Hummm, TextField_Summary is not consistent with what I expected.

Did you actually try it? Is the document you referenced TextFiels_Summary is the correct shape type for fields in Summary display type? I expect the shape to be TextField not TextField_Summary.

@mroskamp
Copy link
Contributor Author

@MikeAlhayek, the issue is specifically with the terminology in commit c5ed897, so it only comes into play when dealing with display modes. I agree that the documentation is correct when not using a display mode, as in your example.

I have tried this and can confirm that including the display type in the shape type does not work, so the documentation should be corrected.

The documentation states:

The Shape must include the display type. For example TextField_Display

As you said, Summary and Detail are display types. Display is not a display type, so what is it?

The documentation also provides this pattern shape type:
[ContentPart]-[ContentField]_[DisplayType]__[DisplayMode]

Using the example in the documentation, I add a "heading" display mode to the TextField. I have a content part Blog with a MyField TextField using the Heading display mode.

Using the documented pattern, this would mean the shape type should be Blog-MyField-TextField_Detail__Header or Blog-MyField-TextField_Summary__Header, depending on the display type.

This is incorrect.

The correct shape type is Blog-MyField-TextField_Display__Header. This matches the actual example in the documentation.

The issue is the pattern in the documentation ([ContentPart]-[ContentField]_[DisplayType]__[DisplayMode]), incorrectly suggests the display type is part of the shape type.

It would be more accurate to use this pattern in the documentation: [ContentPart]-[ContentField]_Display__[DisplayMode].

I can submit a PR for that change, but that doesn't capture that either _Display__ or _Edit__ would presumably work, depending on whether you're placing the editor or the display.

@mroskamp mroskamp linked a pull request Jan 22, 2025 that will close this issue
@MikeAlhayek
Copy link
Member

Please submit a PR to correct the docs. Just be sure to test out the updated docs so that the example you'll provide will be valid

@sebastienros sebastienros added this to the 3.x milestone Jan 23, 2025
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

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

Successfully merging a pull request may close this issue.

3 participants