search

w3c / vc-data-model

W3C Verifiable Credentials v2.0 Specification
https://w3c.github.io/vc-data-model/
Other
290 stars 106 forks source link

Proper rendering of sha-512 values #1443

Closed iherman closed 7 months ago

iherman commented 7 months ago

In [§B.2](Implementations that depend on RDF vocabulary processing) the SHA3-512 values did not display correctly; the values were not rendered as a <code>. This PR removes the <wbr> tag which seems to have created problems in the respec->html conversion.

Preview | Diff

TallTed commented 7 months ago

This appears to me to be one of the places where mixing HTML and Markdown tagging doesn't work so well.

These snippets look fine without the <wbr> tags, but there's a space in each of the sha3-512 values, which I think should not be there, and which I think will cause problems with anyone who's expecting these values to be accurate ... and if they're not expected to be accurate, why expend the effort to calculate them?

I think that the <wbr> tags should be restored, and the linefeed that follows each of them should be removed. Probably the backticks wrapping the sha3-512 values should also be replaced by actual <code> tags.

If restoring the <wbr> tags without the following linefeeds and with the <code> tags replacing the backtick wrappers still results in a ReSpec issue, that demands an issue be raised against ReSpec.

iherman commented 7 months ago

I think that the <wbr> tags should be restored, and the linefeed that follows each of them should be removed. Probably the backticks wrapping the sha3-512 values should also be replaced by actual <code> tags.

You are right, @TallTed. Restoring <wbr> and using <code> seems to work properly, and copying the SHA value does not add any extra space. Thanks. Changed it in https://github.com/w3c/vc-data-model/pull/1443/commits/e56dfaa11ad1aeff065110c8d0451af58efc22da.

decentralgabe commented 7 months ago

@iherman is this ready to merge? if so, will do so after your OK

iherman commented 7 months ago

As far as I am concerned yes, it can be merged

decentralgabe commented 7 months ago

editorial approval, open >1 week. changes addressed. merging