Skip to content

Recode graph diagrams #786

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

Merged
merged 6 commits into from
Aug 25, 2021
Merged

Recode graph diagrams #786

merged 6 commits into from
Aug 25, 2021

Conversation

chaals
Copy link
Contributor

@chaals chaals commented Aug 3, 2021

See #777 - I diverged from that issue and cleaned up the code and thought about the accessibility. (I'll write a different PR that should address #777 directly)

  • make text into actual text

  • use stylesheet

  • simplify source for legibility

  • Order similar to the flow, for accessibility

  • Make credential-graph.svg a subsection, and note with a comment.

  • Add slight background colouring to further delineate each subsection

chaals added 2 commits August 3, 2021 11:06
- make text into actual text
- use stylesheet
- simplify source for legibility
- Order similar to the flow, for accessibility

- Make credential-graph.svg a subsection, and note with a comment.
- Add slight background colouring to further delineate each subsection
@TallTed
Copy link
Member

TallTed commented Aug 3, 2021

Overall, this is a big improvement.

One change I see still needed -- In both diagrams, the yellow and purple clusters within the Credential Graph should each be wrapped in a dotted line which is labeled according to what the colors are meant to communicate -- because the color is the only indicator here.

@chaals
Copy link
Contributor Author

chaals commented Aug 3, 2021

@TallTed said:

the yellow and purple clusters within the Credential Graph should each be wrapped in a dotted line which is labeled

Yeah, I had a similar idea. I was tinking I would just wrap the yellow stuff - the claim - in a box labeled claim because that is one part of a credential. That would take me about 5 minutes ...

I'm not sure what the real process is here, but I'm happy to follow it if someone will enlighten me ;)

chaals added 2 commits August 4, 2021 02:31
As per comment from @TallTed
change font characteristics for graph-labels to distinguish them

(and make the stylesheet more explicitly obvious)
@chaals
Copy link
Contributor Author

chaals commented Aug 4, 2021

I waffled:

I was tinking I would just wrap the yellow stuff - the claim

Done in the latest commits for this PR

@brentzundel brentzundel added errata Erratum for a W3C Recommendation v1.1 editorial Purely editorial changes to the specification. labels Aug 4, 2021
@TallTed
Copy link
Member

TallTed commented Aug 4, 2021

Much improved. Related text might still need some tweaking, and we'll need the full image description for visually impaired readers.

@chaals
Copy link
Contributor Author

chaals commented Aug 6, 2021

The alt text should be the same. Although I can embed it in the diagram as well, for people looking at it standalone.

And yeah, cleaning the text that refers to it is what #777 is actually about - and apparently that's assigned to me now. :S I'll get onto it this week.

This makes credentialgraph.svg and presentation-graph.svg more accessible as standalone diagrams.
@chaals
Copy link
Contributor Author

chaals commented Aug 11, 2021

This PR is IMHO ready to merge (or decide there is something wrong with it as it stands). It's a standalone cleanup of two of the diagrams.

@brentzundel what's the process here? Do I need to do anything more, or can I hand this over, and go work on #777?

@brentzundel
Copy link
Member

@chaals thank you for the PR!
Unless feedback comes in that requests changes, there is nothing more for you to do here. Our process includes a pretty long review period, so the soonest we will be able to merge is in 9 days.

@chaals
Copy link
Contributor Author

chaals commented Aug 11, 2021

Sounds great to me - thank you.

@msporny
Copy link
Member

msporny commented Aug 11, 2021

Only concern I have here is that the original Google Diagrams were not updated, so if we go to re-generate again, those won't have the changes and @chaals work will be overwritten... Originals are here:

https://drive.google.com/drive/folders/0BwQ7X2CScoA-emRfSmVfSU4xNTg?resourcekey=0-4FUIwgBsUkLoCEvfqS0COw

update desc content as request by @TallTed
@chaals
Copy link
Contributor Author

chaals commented Aug 11, 2021

@msporny

the original Google Diagrams were not updated

you mean I should move these to the google drive too, or that you might make further changes in another tool and break the nice SVG code, so I should track those changes elsewhere?

@wyc
Copy link
Contributor

wyc commented Aug 11, 2021

Looking forward to figuring out a way to get these fixes in, while keeping it maintainable, hopefully through the source drawing tools.

@msporny
Copy link
Member

msporny commented Aug 11, 2021

you mean I should move these to the google drive too, or that you might make further changes in another tool and break the nice SVG code, so I should track those changes elsewhere?

I mean "might make further changes in another tool and break the nice SVG code"... :( -- which makes me very sad, because I want to figure out a way to integrate your hard work into the spec.

For now, we could merge the SVG, but I don't know how to move your updates to Google Draw...

@iherman
Copy link
Member

iherman commented Aug 12, 2021

The issue was discussed in a meeting on 2021-08-11

  • no resolutions were taken
View the transcript

3.3. Recode graph diagrams (pr vc-data-model#786)

See github pull request #786.

Wayne Chang: someone has done an angelic job fixing this and making it more readable. Grateful to Chaals for this.

Manu Sporny: I'm afraid that he might have updated auto-generated code. It is really good work, but it may be overwitten
… not sure to backport this into the google draw program we used.

David Chadwick: tallted Done

Wayne Chang: I'll let the discussion continue, but will add a comment about needing to incorporate it into the source image

Ted Thibodeau Jr.: If we're not using this sort order ... https://github.com/w3c/vc-data-model/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-asc

David Chadwick: *kdenhartog Yes I replied to his email rather than going to git

@chaals
Copy link
Contributor Author

chaals commented Aug 13, 2021

Seems like I should keep a copy of these, and if you use a tool to update them with an inaccessible version I should just make those changes myself. I don't use google drawings at all, and don't know if it can keep the accessibility features that are currently there r if it trashes everything. (I would expect it to just make bad edits - which I can fix, and the more atomic they are the easier it is to do so).

(Note that to make these better, at some point I will add some aria into them).

@msporny msporny merged commit 10c3244 into w3c:main Aug 25, 2021
@iherman
Copy link
Member

iherman commented Aug 26, 2021

The issue was discussed in a meeting on 2021-08-25

  • no resolutions were taken
View the transcript

4.3. Recode graph diagrams (pr vc-data-model#786)

See github pull request #786.

Manu Sporny: unfortunately the next time we update the diagrams it will overwrite the good changes the chaals did
… there is not a good solution to this because the diagrams are in google.docs but it does not support his changes

Manu Sporny: +1 to merge and face consequences

Manu Sporny: either it will be difficult to update the diagrams or difficult for some people to read them.

Brent Zundel: is anyone opposed to merging this now?

David Chadwick: There was no opposition so Manu performed the merge

@chaals
Copy link
Contributor Author

chaals commented Aug 26, 2021

Please do ping me if you want to update the diagrams, so I have a chance to make the changes manually.

(You could also do that in some tool that handles SVG well enough not to break the original source code, as that would simplify the update process for me...)

@kdenhartog
Copy link
Member

Given this PR is slightly different than the original issue that raised it (#777) I've opted to leave the Editorial and Errata labels on the PR directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Purely editorial changes to the specification. errata Erratum for a W3C Recommendation merge after 14 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants