Extend dl with div wrapped groups #89
Conversation
…oposed solution is a workaround the current requirement for ElementDefinition.
There was a problem hiding this comment.
Thanks for this @nkalvi, and sorry for taking so long to review your PR. Left a few comments inline.
Sources/Plot/API/HTMLElements.swift
Outdated
| /// This is allowed according to the HTML spec: // https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element | ||
| /// | ||
| /// - parameter nodes: The element's attributes and child elements (`<dl>` or `<dd>` elements). | ||
| static func dlDiv(_ nodes: Node<HTML.DescriptionListContext>...) -> Node { |
There was a problem hiding this comment.
Not quite sure why we can't use div here? You mentioned that it being a requirement of ElementDefinition, but I don't really understand why that has something to do with this extension? Mind clarifying?
There was a problem hiding this comment.
Thanks for reviewing this - I cannot remember what error I was getting related to ElementDefinition while working on this. I will get back to this if I see or remember the situation.
There was a problem hiding this comment.
Please wait on this pull request. This is what I get when I changed from dlDiv to div:
Type 'ElementDefinitions.Div' does not conform to protocol 'ElementDefinition'
I will see what can be done without adding a new method.
There was a problem hiding this comment.
Hi John, I'm not sure how to accomplish this without a defining a new function for the context.
What I cannot understand is, if I add Plot as a dependency and the following in my app, it works as expected;
public extension Node where Context == HTML.DescriptionListContext {
// The `<div>` element wraps a group, that is part of a term-description group in a description list (`<dl>` element).
//
// This is allowed according to the HTML spec: // https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element
/// - parameter nodes: The element's attributes and child elements (`<dl>` or `<dd>` elements).
static func div(_ nodes: Node<HTML.DescriptionListContext>...) -> Node {
.element(named: "div", nodes: nodes)
}
}but when I try to add this to Plot, it fails because, Node.div becomes ambiguous in
public enum Div: ElementDefinition { public static var wrapper = Node.div }Since adding it inside the app works for my purposes (I would love to know why though), I can scrap this pull request.
There was a problem hiding this comment.
Thanks for the update, @nkalvi! Let me look into this a bit later in the week, and I'll let you know what I'll find 👍
Sources/Plot/API/HTMLElements.swift
Outdated
| public extension Node where Context == HTML.DescriptionListContext { | ||
| /// The `<div>` element wraps a group, that is part of a term-description group in a description list (`<dl>` element). | ||
| /// | ||
| /// This is allowed according to the HTML spec: // https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element |
There was a problem hiding this comment.
This is a great sentence to add to the commit message, but I don't think it needs to be a part of the public documentation.
There was a problem hiding this comment.
I agree. I have changed it.
JohnSundell
added
the
awaiting update
The `<div>` element wraps a group, that is part of a term-description group in a description list (`<dl>` element). This is allowed according to the HTML spec: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element
Add tests for `div` inside description lists.
Fix typo
JohnSundell
added
needs investigation
Base the `Node.div` API on a new `HTMLDividableContext` protocol, which enables that API to be shared between `HTML.BodyContext` and `HTML.DescriptionListContext`. While this does come with a minor downside of now requiring an explicit context whenever a `div` node is created as a stand-alone value (when using `Node.div` directly, without defining any child nodes within it), in practice that should be very rare, as `div` nodes are most likely placed within a known context (such as within a `body` node, or another node that inherits `HTML.BodyContext` from its parent).
There was a problem hiding this comment.
Figured out what was wrong, @nkalvi. The problem was that, by adding another div overload, the Node.div reference used within the Div element definition became ambiguous, which in turn made the compiler not consider that class to conform to the ElementDefinition protocol. I solved that problem by extracting the div API to a protocol, which is conformed to by both HTML.BodyContext and HTML.DescriptionListContext. Updated your PR with a commit containing those changes, so this PR is now ready to be merged! Thanks a lot for contributing 👍
JohnSundell
removed
the
needs investigation
|
Very nice John! Thank you. |
According to the HTML spec: // https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element
The proposed solution (using
dlDivinstead ofdiv) is a workaround for the current requirement forElementDefinition.