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

Table of contents clarifications #254

Merged
merged 6 commits into from
Jun 29, 2018
Merged

Table of contents clarifications #254

merged 6 commits into from
Jun 29, 2018

Conversation

mattgarrish
Copy link
Member

@mattgarrish mattgarrish commented Jun 28, 2018

This PR addresses the concern I raised in #251 by removing the fragment identifier for table of contents, as recommended by @iherman.

In trying to rewrite the section, I also noticed that we were mixing a lot of toc construction requirements into the property definition, when all the property does is identify the resource containing the toc. To fix this, I've created a new subsection under "Creation" to detail how to make the table of contents, and rewritten the infoset to specify how to obtain the link.

Of note is that it disallows a fragment identifier on PublicationLinks, and also explicitly disallows a resource from being listed in both the resource list and default reading order, or listed two or more times within either of those lists.

Comments welcome.


Preview | Diff

Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

  • I have made a change in 4.6.2:
    • the property name is 'url' and not href
    • the link (ie, url) is not leading to the ToC but the resource containing it (just to be precise)
  • I have (now...) a problem disallowing fragment id-s altogether: I am fine when we refer to resources within the WP (eg, reading order or resources) but I do not think we can restrict values in links (ie, extra links) which are largely URL-s that the authors do not really control. An example from W3C: the 'official' URL for the W3C copyright is http://www.w3.org/Consortium/Legal/ipr-notice#Copyright; this means adding a 'copyright' link as part of links (which would be perfectly justified) could not be added if this restriction is in effect.

I know we are a bit back to square one, and I we should give some thoughts on how to handle that...

@iherman
Copy link
Member

iherman commented Jun 29, 2018

@mattgarrish, on the second item (if you agree) I believe the cleanest approach is:

  • in 4.3.1. remove the restriction on the value of url referring to the fragment identifier
  • both in 4.7.1.1. and 4.7.2.1. add a restriction explicitly in the text, saying something like "URL-s identifying resources MUST NOT contain fragment strings".

I do not want to make the change without your agreement, though.

@mattgarrish
Copy link
Member Author

but I do not think we can restrict values in links (ie, extra links) which are largely URL-s that the authors do not really control.

Funny, I had the same thought last night as I was falling asleep (thinking of wpub is better than counting sheep!).

I was thinking that we make the fragment conditional in the PublicationLink definition, though, to avoid having to repeat it elsewhere. For example:

A URL [[!url]]. The URL MAY contain a fragment identifier only when used with ???

(Do we call that section "links" now, so we have to say "links in the links property"?)

@llemeurfr
Copy link
Contributor

llemeurfr commented Jun 29, 2018 via email

@iherman
Copy link
Member

iherman commented Jun 29, 2018

WP as sheep:-)

There are some issues that may lead to other 'links' (e.g., links of images, stuff like that), where similar restrictions may apply. Also, we may end up using a schema.org object later to replace it (pending some issues to be discussed with DanBri & Co).

All this to say that It may be more future safe to add that remark to the individual items rather than the overall definition of PublicationLink imho.

@HadrienGardeur
Copy link

+1 to what @iherman suggested: these restrictions apply to the collection (resources and readingOrder) rather than the link model.

@mattgarrish
Copy link
Member Author

may be more future safe to add that remark to the individual items rather than the overall definition of PublicationLink

It's just a painful approach as the links property is the lone anomaly.

But I'm not going to get worked up over it. It might just also be good to make clear in the publicationlink definition that people should refer to each property definition for possible additional restrictions.

@HadrienGardeur
Copy link

@mattgarrish from a JSON-LD perspective, links is the only place where fragments make sense, but for the WebIDL we'll also have fragments in toc as well (and potentially other lists/navigation, such as page list or landmarks).

@iherman
Copy link
Member

iherman commented Jun 29, 2018

@mattgarrish

But I'm not going to get worked up over it. It might just also be good to make clear in the publicationLink definition that people should refer to each property definition for possible additional restrictions.

Big +1 to that.

Will you do these changes?

@mattgarrish
Copy link
Member Author

Yup, just getting the little one off and then will push a fix.

@mattgarrish
Copy link
Member Author

I've pushed a change to spell out the restriction in each section. (I also tweaked the "Extra resources" section to use "Links", but will return to that later.)

@iherman
Copy link
Member

iherman commented Jun 29, 2018

@mattgarrish this should be merged, as far as I am concerned...

@mattgarrish mattgarrish merged commit 039f93a into master Jun 29, 2018
@mattgarrish mattgarrish deleted the toc-clarifications branch June 29, 2018 14:12
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