-
Notifications
You must be signed in to change notification settings - Fork 1
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
Recipe #466: Sharing a link to open a Manifest in a specific viewer. #125
Comments
I approved the recipe, but I have a comment about some of the URLs. For example: The URL syntax may be consulted in rfc 1808, section 2.2. If the values of URL parameters are not encoded, in some cases there are characters that will "confuse" URL parsing.
|
There are some missing back ticks for quoted literals in the recipe, e.g.:
I think the current sentence is confusing:
in that we are talking only about about the URI reference case and don't otherwise mention JSON objects. Also, the "it" (the manifest URI) is not very clear. It probably is good to mention that the Content State spec provides for more complex reference to state using JSON objects rather than just omitting. Perhaps something like:
I do not think that it is necessary to encode the |
Thanks for calling out the encoding issue @nfreire! Per the spec, for HTTP GET requests:
So the links provided in the recipe aren't correct as written, they should be encoded as base64. So 👎 from me until that gets fixed. |
I'm not sure it should be base64 encoded. In section 6 it says: "When the content state is a plain URI, rather than a JSON object, it must not be content-state-encoded." https://iiif.io/api/content-state/1.0/#62-content-state-encoding-and-uri-requirements also in the following section: https://iiif.io/api/content-state/1.0/#initialization-mechanisms-link The simple URL example is not encoded but the JSON version explicitly mentions it should be content-state-encoded. |
Yes, you're right @glenrobson. I think that's a bug in the spec :( It should have some sort of explicit encoding requirement, otherwise we'll end up applying the wrong number of encoding/decoding pairs. At which point the recipe conforms to the spec ... but I think the spec needs to be changed! So I'm overall -1 on publishing it, through no fault of the recipe authors. |
It feels like a step back not to be able to do: |
The value of notes :) The encoding of URIs issue: IIIF/discovery#90 Then: https://docs.google.com/document/d/1YZtE7OCGD5bEO52Xa_o-aE40Z9JkJ8awE1j43-rGZuU/edit
I guess I shouldn't have missed that meeting! Currently it's undetermined how many times --> IIIF/api#2292 |
Wrt. encoding I think both the spec and the recipe are OK, but could be improved by some additional mention. To get really into the weeds, RFC3986 URI syntax says the following about what characters can appear in the query component of the URI:
where
So, the query component can explicitly include “/”, “:”, "-" and a number of other characters that a brute-force URL encoding routine may encode. The URI spec also notes that as “one frequently used value is a reference to another URI, it is sometimes better for usability to avoid percent-encoding those characters.” I thus maintain that the two examples in the recipe:
are best left as-is, without any additional encoding (and we note they do also work fine, which is reassuring). |
if const url1 = "https://example.org/manifest/%2550";
const url2 = "https://example.org/manifest/%50";
const encoded1 = encodeURIComponent(url1);
const encoded2 = encodeURIComponent(url2);
const searchParams = new URLSearchParams(`a1=${encoded1}&a2=${encoded2}`);
const out1 = searchParams.get("a1");
const out2 = searchParams.get("a2");
console.log(out1 === url1); // true
console.log(out2 === url2); // true |
@zimeon |
Agreed as well. |
Issue 125 (Recipe #466: Sharing a link to open a Manifest in a specific viewer.)+1: 13 [JulieWinchester akrishnan15 cubap glenrobson julsraemy markpatton mikeapp nfreire regisrob rentonsa robcast tpendragon zimeon] Result: 13 / 13 = 1.00Super majority is in favor, issue is approved |
Links
Background and Summary
This recipe is the first one that covers the IIIF Content State API. This is the most basic version of the use of Content state which opens up a Manifest in a specific viewer. Once the basic one is complete hopefully we can move on to opening a specific canvas in a viewer and other recipes.
Voting and changes
We welcome comments on the recipe and as well as voting +1, confused face or -1 feel free to add comments to this issue. If this issue is approved then the author will take account of the comments before we merge the branch in to the master cookbook branch.
If the recipe is rejected by the TRC then we will make the changes requested and resubmit it to a future TRC meeting. If you feel that your comments are substantial enough that the recipe should be looked at again by the TRC after the changes have been made please vote -1 (thumbs down). A confused face is treated as abstaining.
Changes to the recipe will only be made after the TRC voting process has concluded.
The text was updated successfully, but these errors were encountered: