feat(core/dfn): add `data-defines` on some patterns of term/definition association #4620

dontcallmedom · 2023-12-21T14:30:25Z

As discussed in #4522
See also w3c/reffy#1444

As discussed in speced#4522 See also w3c/reffy#1444

src/core/dfn.js

Co-authored-by: François Daoust <[email protected]>

src/core/dfn.js

marcoscaceres · 2023-12-22T04:00:56Z

I'm not loving dfn-desc to be honest... it's not as author friendly as it could be and not in line with other CSS named we use in bikeshed and respec (e.g., <dl class="algorithm">). Could we just use "definition" instead:

<p class="definition">
   This is a <dfn>term</dfn> that is being defined.
</p>

<p>
   This is some term <dfn>term</dfn> that is being defined.
   And <span class="definition">this is some <dfn>other</dfn> that is being defined</span>
</p>

We can still use data-dfn-desc as the thing, but ReSpec should add that... that's what we do for class="export": it becomes data-export="" after processing.

src/core/dfn.js

marcoscaceres

Suggested some changes...

https://github.com/w3c/respec/pull/4620#discussion_r1434706130 and https://github.com/w3c/respec/pull/4620#issuecomment-1867208920

src/core/dfn.js

sidvishnoi · 2023-12-22T11:03:15Z

tests/spec/core/dfn-spec.js

+            <dt><dfn>different convention</dfn></dt>
+            <dd id="desc2">Different conventions can be applied to associate a term with its description</dd>
+          </dl>
+          <dl>


Thinking out loud:
Do we want/need to handle the cases with multiple dt or multiple dd?
Multiple dt: as an alias.. sounds like job for data-lt
Multiple dd: why?

tests/spec/core/dfn-spec.js

As suggested in https://github.com/w3c/respec/pull/4620#discussion_r1435000012

sidvishnoi · 2023-12-23T10:30:34Z

src/core/dfn.js

+  // A dl with class hasdefinitions marks all next siblings of dt with the class
+  // definition
+  /** @type NodeListOf<HTMLElement> */
+  const describedDTs = document.querySelectorAll("dl.hasdefinitions dt");


Maybe dl.definitions?

the reason I was liking "hasdefinitions" more than just "definitions" is that it is more easily distinguishable from the other pattern (it's easy to see that "hasdefinitions" is different from "definition", but it would really easy to confuse "definition" and "definitions" which generate quite different behavior).

Makes sense, but still feels a little odd to me. Maybe I can get used to it :)

Wasn't loving hasdefinitions either tbh. I updated the PR to use "definitions".

src/core/dfn.js

sidvishnoi · 2023-12-23T10:36:16Z

src/core/dfn.js

+    let curEl = el;
+    let dfn;
+    while (curEl) {
+      dfn = curEl.querySelector("dfn[data-dfn-type]");


A comment explaining why [data-dfn-type] would be be nice.

sidvishnoi

LGTM 🎉

Just one nit: dl.hasdefinitions should probably become dl.definitions.

marcoscaceres · 2023-12-28T05:19:25Z

src/core/dfn.js

+
+// - Sets data-defines on well-known definition content patterns
+function addDefinitionPointers() {
+  // A dl with class hasdefinitions marks all next siblings of dt with the class


I don't dislike this (dl.definitions), but I feel this is overstepping based on what we have previously discussed. I still think we should start with an extremely simple / basic system and build it up very incrementally (and eventually get to this, but I don't think we are there yet).

a <dl> containing definitions is exactly what the VC spec (our initial customer) is using AFAICT; if we're not going to support that convention, I'm not sure we need a respec patch at all at the moment (since reffy recognizes data-defines without any additional help).

Right, and that might be ok. But let's settle on the convention for:

container (p, span) has .definition

and multiple: dl.definitions

And explicitly marked as such.

tests/spec/core/dfn-spec.js

Based on review comments https://github.com/w3c/respec/pull/4620#discussion_r1437398798 https://github.com/w3c/respec/pull/4620#discussion_r1437393883

marcoscaceres

I think this is good as a start.

We might need to deal with edge cases like:

<p class="definition">
  <dfn>a</dfn> ... <dfn>b</dfn>
</p>

src/core/dfn.js

tests/spec/core/dfn-spec.js

dontcallmedom · 2024-01-26T08:55:44Z

is there anything blocking this or should I go ahead and merge it at this point?

sidvishnoi · 2024-01-26T09:33:40Z

OK by me. @marcoscaceres?
Let me know I'll merge and release.

sidvishnoi · 2024-01-29T09:55:50Z

Going ahead with the merge given Marcos approved already.

Add data-defines on well-known patterns of term/definition association

82a3cf3

As discussed in speced#4522 See also w3c/reffy#1444

dontcallmedom requested a review from tidoust December 21, 2023 14:30

Fix lint

5150f01

tidoust reviewed Dec 21, 2023

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

src/core/dfn.js Outdated Show resolved Hide resolved

src/core/dfn.js Outdated Show resolved Hide resolved

src/core/dfn.js Outdated Show resolved Hide resolved

Fix typo

4204017

Co-authored-by: François Daoust <[email protected]>

dontcallmedom commented Dec 21, 2023

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

Use data-dfn-type consistently as a dfn selector

2ca26dd

tidoust approved these changes Dec 21, 2023

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

dontcallmedom requested a review from sidvishnoi December 21, 2023 16:14

marcoscaceres reviewed Dec 22, 2023

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

marcoscaceres requested changes Dec 22, 2023

View reviewed changes

Address review feedback

6a8fe49

https://github.com/w3c/respec/pull/4620#discussion_r1434706130 and https://github.com/w3c/respec/pull/4620#issuecomment-1867208920

dontcallmedom requested a review from marcoscaceres December 22, 2023 07:47

sidvishnoi reviewed Dec 22, 2023

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

sidvishnoi reviewed Dec 22, 2023

View reviewed changes

tests/spec/core/dfn-spec.js Outdated Show resolved Hide resolved

sidvishnoi changed the title ~~Add data-defines on well-known patterns of term/definition association~~ feat(core/dfn): add data-defines on some patterns of term/definition association Dec 22, 2023

dontcallmedom added 2 commits December 22, 2023 17:02

Use more intuitive pattern to associate descriptions to their terms

246b81a

As suggested in https://github.com/w3c/respec/pull/4620#discussion_r1435000012

Fix lint

075bba5

sidvishnoi reviewed Dec 23, 2023

View reviewed changes

src/core/dfn.js Show resolved Hide resolved

sidvishnoi reviewed Dec 23, 2023

View reviewed changes

sidvishnoi approved these changes Dec 23, 2023

View reviewed changes

marcoscaceres reviewed Dec 28, 2023

View reviewed changes

tests/spec/core/dfn-spec.js Outdated Show resolved Hide resolved

Revert to use only dl.hasdefinitions and in-element dfn detection

d2e169a

Based on review comments https://github.com/w3c/respec/pull/4620#discussion_r1437398798 https://github.com/w3c/respec/pull/4620#discussion_r1437393883

dontcallmedom requested a review from marcoscaceres January 8, 2024 14:42

marcoscaceres approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into dfn-prose

37bfb00

marcoscaceres reviewed Jan 10, 2024

View reviewed changes

src/core/dfn.js Outdated Show resolved Hide resolved

marcoscaceres reviewed Jan 10, 2024

View reviewed changes

tests/spec/core/dfn-spec.js Outdated Show resolved Hide resolved

Apply suggestions from code review

a769813

sidvishnoi merged commit 390d8b6 into speced:main Jan 29, 2024
8 checks passed

dontcallmedom mentioned this pull request Feb 2, 2024

Contextualizing definitions for export #4522

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core/dfn): add `data-defines` on some patterns of term/definition association #4620

feat(core/dfn): add `data-defines` on some patterns of term/definition association #4620

dontcallmedom commented Dec 21, 2023

marcoscaceres commented Dec 22, 2023 •

edited

Loading

marcoscaceres left a comment

sidvishnoi Dec 22, 2023

sidvishnoi Dec 23, 2023

dontcallmedom Jan 8, 2024

sidvishnoi Jan 8, 2024

marcoscaceres Jan 10, 2024

sidvishnoi Dec 23, 2023

sidvishnoi left a comment

marcoscaceres Dec 28, 2023 •

edited

Loading

dontcallmedom Jan 8, 2024

marcoscaceres Jan 10, 2024

marcoscaceres left a comment

dontcallmedom commented Jan 26, 2024

sidvishnoi commented Jan 26, 2024

sidvishnoi commented Jan 29, 2024

feat(core/dfn): add data-defines on some patterns of term/definition association #4620

feat(core/dfn): add data-defines on some patterns of term/definition association #4620

Conversation

dontcallmedom commented Dec 21, 2023

marcoscaceres commented Dec 22, 2023 • edited Loading

marcoscaceres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidvishnoi left a comment

Choose a reason for hiding this comment

marcoscaceres Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres left a comment

Choose a reason for hiding this comment

dontcallmedom commented Jan 26, 2024

sidvishnoi commented Jan 26, 2024

sidvishnoi commented Jan 29, 2024

feat(core/dfn): add `data-defines` on some patterns of term/definition association #4620

feat(core/dfn): add `data-defines` on some patterns of term/definition association #4620

marcoscaceres commented Dec 22, 2023 •

edited

Loading

marcoscaceres Dec 28, 2023 •

edited

Loading