Closed iherman closed 1 year ago
The one thing I'll note is that it's really hard to read in dark mode, a sample:
Hm. That is strange.
The generated SVG file has a <svg ... style="background-color:#fff"
in it, which should create a white background. (This is the result of draw.io, not my making.) I would have expected the browser takes this into account. Or does dark mode changes all the colors (I see the property names displayed in white; my setting was dark gray). If so, I do not have the faintest idea how that can be controlled...
@dlongley how did you get to dark mode? I have set my browser (chrome based) and went to the preview and displayed the vocabulary file and did not come out as dark mode...
@iherman,
My default browser uses "Dark mode for web contents" (chrome://flags/#enable-force-dark
). For a similar issue and an image that got fixed to address it, see: https://github.com/w3c/rdf-canon/pull/152#pullrequestreview-1550758135
@dlongley I will look into this. The situation is different because the SVG file has active links, and those should be working both when the content is in the html file and when rendered separately.
Would it be ok if, from your point of view, this PR could be merged, and I would treat this as a possible after CR improvement? It may take longer to solve this.
@dlongley @msporny I believe it works now with dark mode, too. I have essentially removed most of the colors, which is probably better for accessibility in general, too.
Thanks, @iherman, it looks great!
I'll note that I don't see
created
in the property list forProof
(it should be there along withexpires
,nonce
, etc.).
Shoot... Actually, it is/was worse: the term was missing from the vocabulary altogether! Added now.
I also made a tiny addition to the diagram by (artificially) showing a specific connection between the Graph with what it contains.
I also made a tiny addition to the diagram by (artificially) showing a specific connection between the Graph with what it contains.
That black-dotted-line for contains
should be added to the key, as it otherwise seems to be a mistake that might be meant to be read as either Domain
or Range
even though it says contains
.
I suggest also changing the line style of Domain
, to be the same blue and arrow (from Property
-to-Class
) as Range
, but use a different line-style (like dash-dot-dash, or dot-dot) than Range
(which can stay as dash-dash). These are currently doubly confusing, as their diamond-head is on the "target" (Class
) end in the diagram, but on the "source" (Property
) end in the key.
Last, I suggest reversing the direction of the Subclass
arrows (or changing the key label to Subclass of
or Superclass
and keeping the current direction).
I also made a tiny addition to the diagram by (artificially) showing a specific connection between the Graph with what it contains.
That black-dotted-line for
contains
should be added to the key, as it otherwise seems to be a mistake that might be meant to be read as eitherDomain
orRange
even though it sayscontains
.
I agree.
I suggest also changing the line style of
Domain
, to be the same blue and arrow (fromProperty
-to-Class
) asRange
, but use a different line-style (like dash-dot-dash, or dot-dot) thanRange
(which can stay as dash-dash). These are currently doubly confusing, as their diamond-head is on the "target" (Class
) end in the diagram, but on the "source" (Property
) end in the key.
Just to be clear, what you would like to see is sg like
Domain Class -- . -- . --- > Property Box - - - - - > Range Class
where the domain and range arrows would have the same color by different style? The only disagreement I have is the same color aspect. The line types in the tool I use may not provide enough differentiation, and having different colors may help.
Otherwise, I agree.
Last, I suggest reversing the direction of the
Subclass
arrows (or changing the key label toSubclass of
orSuperclass
and keeping the current direction).
Agree. I will change the key label.
@TallTed : procedural question: is it o.k. if we merge this PR (which also contain vocab changes) and postpone the new diagram generations slightly (this also includes your comment on the VCDM diagram). The reason is timing: the changes I had to do to make the diagrams working on dark mode led to a small problem: while all the boxes on the diagram are also active links to their respective definition (which I think is a good thing) the changes I used destroyed that (you can only click on the borders of the shapes). I have found a way to overcome that in the tool I use, but it necessitates a complete re-generation of the figures, which takes some time (and I will be off next week). I would not want to hold this PR in the air for too long.
Cc @msporny
@iherman
Domain Class -- . -- . --- > Property Box - - - - - > Range Class
Something like that, yes, ensuring that the arrowheads (no more diamonds) in the key match the directionality in the diagram. Since Property has Domain and Range (neither Domain nor Range has Property), the arrows should both point from Property, so
Domain Class < -- . -- . -- Property Box - - - - - > Range Class
I can see how the different colors could be useful, so will not argue with keeping them.
is it o.k. if we merge this PR (which also contain vocab changes) and postpone the new diagram generations slightly
Sure. Since the diagram will then be done some days later, I request a minimally detailed tracker issue be opened, just to ensure it doesn't get lost in the shuffle.
@TallTed see #175
Editorial, multiple reviews, changes requested and made, no objections, merging.
Now that most of the DI related PR-s are merged, I did a sync step for the vocabulary by updating it. In particular:
Proof
class is now defined through the spec (as it should)proof
property is defined through https://www.w3.org/TR/vc-data-integrity/#proof-sets section; the comment field has been removed (a specific<dfn>
for the property might be better, but this is also working).Key
class has been set as 'deprecated' (there is no mention to it in the spec!)Ed25519VerificationKey2020
is now a subclass ofsec:VerificationMethod
, which is in line with how it is used in the eddsa spec (the previous setting was probably buggy)sec:cryptosuiteString
datatype has been added to the vocabularyI have also finalzied and added a diagram for the vocabulary, along the same lines as https://github.com/w3c/vc-data-model/pull/1236. Hopefully, this will help locating possible errors in the vocabulary. Note that the long description for the diagram is missing; I will do that when there is a consensus as for the content of the diagram itself.
As usual, the preview for the result is also available here (although the vocabulary changes are pretty clear):
https://w3c.github.io/yml2vocab/previews/di/
In particular, the diagram is also directly available here:
https://w3c.github.io/yml2vocab/previews/di/vocabulary.svg