Skip to content

Dispatch render suites on renderSuite, not type. - #76

Merged
dmitrizagidulin merged 4 commits into
mainfrom
suite-render-method
Sep 29, 2026
Merged

dmitrizagidulin merged 4 commits into
mainfrom
suite-render-method

Conversation

@dmitrizagidulin

@dmitrizagidulin dmitrizagidulin commented Aug 25, 2026 •

Copy link
Copy Markdown
Collaborator

From discussions in issues #54 and #55.

  • Renames TemplateRenderMethod to SuiteRenderMethod (keeps optional superclass RenderMethod open, per @iherman's preferences)
  • The card render suite is now the summary render suite.
  • The renderSuite table entry now states the value is a plain JSON string with exact case-sensitive matching, lists data, html, nfc (though see Folding nfc into a static render suite needs a dispatch mechanism #77) as the values this spec defines, and requires implementations to ignore entries with unrecognized values. (See issue Registry for renderSuite values #75 for related discussion)
  • Each suite section got a "Use of Common Properties" table

Preview | Diff

Signed-off-by: Dmitri Zagidulin <dzagidulin@gmail.com>
Signed-off-by: Dmitri Zagidulin <dzagidulin@gmail.com>
Comment thread index.html Outdated
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Comment thread index.html
"type": "TemplateRenderMethod",
"renderSuite": "card",
"type": "SuiteRenderMethod",
"renderSuite": "summary",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right name, but will +1 this PR to keep things moving while we still work out a better name.

@msporny msporny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because it's an iteration in the right direction, not because I agree with the name of the card, or think details such as "template" for NFC makes sense. I presume other PRs will follow that aligns that stuff w/ consensus of the group.

@TallTed

TallTed commented Sep 22, 2026

Copy link
Copy Markdown
Member

I suggest opening tracking issues for each of the reasons (raised by @msporny, @dlongley, and/or thought of by others) why this PR is imperfect, to help ensure that they are indeed addressed in follow-up PRs.

@iherman iherman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section on the former card rendering is inconsistent. It uses summary in the title now, but subsections, objects, etc, are still referred to as card, and, in some examples, we are talking about data.

I am fine using the term summary for now, as part of the more general change proposed here (although the resolution on the final naming is still missing). But we should be consistent.

Signed-off-by: Dmitri Zagidulin <dzagidulin@gmail.com>
@dmitrizagidulin

Copy link
Copy Markdown
Collaborator Author

@iherman I updated the rest of the card usages to summary for consistency.

@dmitrizagidulin

Copy link
Copy Markdown
Collaborator Author

Several approvals, merging for momentum and to continue the discussion.

@dmitrizagidulin
dmitrizagidulin merged commit d00c2f1 into main Sep 29, 2026
2 checks passed
@dmitrizagidulin
dmitrizagidulin deleted the suite-render-method branch September 29, 2026 14:32
@TallTed

TallTed commented Sep 29, 2026

Copy link
Copy Markdown
Member

I see this PR has been merged. I have not seen tracking issues for the concerns with this PR, as mentioned in #76 (comment), as "raised by @msporny, @dlongley, and/or thought of by others". Have those concerns been addressed elsewhere?

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.

5 participants