Editorial: Add a diagram for storage model #152

saschanaz · 2022-11-21T22:20:25Z

Fixes #151

Used SVGO as @annevk suggested, which indeed vastly reduces the size.

saschanaz · 2022-11-21T22:24:51Z

"file:/home/runner/work/storage/storage/storage.spec.whatwg.org/index.html":223.7-223.116: error: An “img” element with no “alt” attribute must not have any “aria-*” attributes other than “aria-hidden”.
"file:/home/runner/work/storage/storage/storage.spec.whatwg.org/index.html":223.7-223.116: error: An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.
"file:/home/runner/work/storage/storage/storage.spec.whatwg.org/commit-snapshots/a976c463f54c26c953f7d6df3a59a5ee146609bc/index.html":232.7-232.116: error: An “img” element with no “alt” attribute must not have any “aria-*” attributes other than “aria-hidden”.
"file:/home/runner/work/storage/storage/storage.spec.whatwg.org/commit-snapshots/a976c463f54c26c953f7d6df3a59a5ee146609bc/index.html":232.7-232.116: error: An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images

It seems the validator blocks any use of ARIA attributes and forces alt? @whatwg/a11y, should I just use alt here instead?

aardrian · 2022-11-22T00:02:48Z

I believe we expected you would embed an SVG inline, as it was in the original issue. All the advice was geared toward that. We did not offer advice for referencing it via <img>.

The <img> element requires the alt attribute to provide its accessible name. Your code uses aria-describedby, which provides an accessible description.

domenic · 2022-11-22T01:54:03Z

It looks like #148 failed to build. Please replace all browsing session references appropriately before merging... I'll investigate. Edit: build is fixed, and "browsing session" references no longer appear on https://storage.spec.whatwg.org/.

saschanaz · 2022-11-22T02:18:20Z

Does it make sense to do <img alt="A storage model diagram" aria-describedBy="model-description"> or should I copypaste the whole paragraph to the alt attribute? 👀

(I'm not against embedding the SVG, it's just that it'll introduce some manual work if the diagram ever changes. I wonder Bikeshed can just import SVG files.)

aardrian · 2022-11-22T02:55:04Z

alt="Storage model diagram" is fine, as long as it accompanies the actual storage model diagram.

Makefile

storage.bs

annevk · 2022-11-22T09:20:45Z

assets/model-diagram.mmd

+"Browsing session" ||--|| "Storage shed" : "holds (session)"
+"Storage shed" ||--o{ "Storage shelf" : "for each storage key"
+"Storage shelf" ||--|| "Storage bucket" : "holds (default)"
+"Storage bucket" ||--|{ "Storage bottle" : "for each storage endpoint"


Shall we lowercase all these terms? I'm not entirely sure, but it looks a little off to me at the moment.

I'm not sure about that either, diagrams tend to have the cases this way.

annevk

It's unfortunate that preview doesn't work for this image. I guess that's due to it only doing a Bikeshed build and not a full build.

@domenic can you do a quick peak? I'd love your thoughts on:

Doing diagrams.
The Node.js stuff here.
Using assets/ for structure.

(Mainly because if this is good, we should adopt it elsewhere, e.g., in Fetch.)

annevk · 2022-11-22T16:35:44Z

.gitignore

@@ -1,3 +1,5 @@
 /storage.spec.whatwg.org/
 /deploy.sh
 /storage.html
+/node_modules/
+package-lock.json


This we'll also have to change upstream: https://github.com/whatwg/spec-factory/blob/main/factory.json. But it's straightforward enough.

Will it override this file if unchanged btw?

I'm unsure on whether or not we should check in package-lock.json. It looks like we don't for other Node-using spec repos. But it's generally good practice. We check it in for participate.whatwg.org.

If you check it in, you get reproducible builds. If you don't check it in, all your dependencies-of-dependencies will get recomputed each time, potentially with inadvertent breaking changes.

However, if you check it in, you also start getting annoying GitHub security alerts about all your dependencies-of-dependencies :-/.

storage.bs

Co-authored-by: Anne van Kesteren <[email protected]>

domenic · 2022-11-23T06:51:16Z

.gitignore

@@ -1,3 +1,5 @@
 /storage.spec.whatwg.org/
 /deploy.sh
 /storage.html
+/node_modules/
+package-lock.json


I'm unsure on whether or not we should check in package-lock.json. It looks like we don't for other Node-using spec repos. But it's generally good practice. We check it in for participate.whatwg.org.

If you check it in, you get reproducible builds. If you don't check it in, all your dependencies-of-dependencies will get recomputed each time, potentially with inadvertent breaking changes.

However, if you check it in, you also start getting annoying GitHub security alerts about all your dependencies-of-dependencies :-/.

domenic · 2022-11-23T06:51:48Z

assets/model-diagram.mmd

+%% Build command: `npm i && npm run build-diagram`
+
+"User agent" ||--|| "Storage shed" : "holds (local)"
+"Browsing session" ||--|| "Storage shed" : "holds (session)"


Please change "Browsing session" to "Top-level traversable", per the latest spec.

A browsing session holds a storage shed, which is a storage shed. A browsing session’s storage shed holds all session storage data.

Err, it's still there, should it be fixed too?

Ahh, that's because the build failure. 😅

domenic · 2022-11-23T06:53:01Z

package.json

@@ -0,0 +1,14 @@
+{
+  "name": "storage",


I suggest moving "private": true to the top, and removing "name" and "version". Compare to https://github.com/whatwg/webidl/blob/main/package.json.

domenic · 2022-11-23T06:53:53Z

assets/model-diagram.svg

@@ -0,0 +1,50 @@
+<svg xmlns="http://www.w3.org/2000/svg" aria-labelledby="chart-title-mermaid-1669067354847 chart-desc-mermaid-1669067354847" style="max-width:396.227px;background-color:#fff;font-family:&quot;trebuchet ms&quot;,verdana,arial,sans-serif;font-size:16px;fill:#333" viewBox="0 0 396.227 815">


Do we want to check in this build output? Or should it be generated on each deploy?

If the latter, we need to do some work to make this repo more like webidl. (Which is another repo which uses Node as part of its build process.) We can make the changes as part of this PR, but we'd also need to update spec-factory so that it generates the files appropriately next time in the same way.

Either way is fine to me, but let's try that...

domenic · 2022-11-23T06:58:04Z

assets/model-diagram.svg

@@ -0,0 +1,50 @@
+<svg xmlns="http://www.w3.org/2000/svg" aria-labelledby="chart-title-mermaid-1669067354847 chart-desc-mermaid-1669067354847" style="max-width:396.227px;background-color:#fff;font-family:&quot;trebuchet ms&quot;,verdana,arial,sans-serif;font-size:16px;fill:#333" viewBox="0 0 396.227 815">


This file will need to be added as an "extra_files in https://github.com/whatwg/spec-factory/blob/main/factory.json otherwise the build will not work. That updates the Makefile like so: https://github.com/whatwg/encoding/blob/main/Makefile#L23 .

We'll need to be sure to exclude all the other files. So maybe something like assets/*.svg?

Alternately, you could have all the other stuff live in scripts/, and have it generate a new file in assets/ or images/. Then you'd add assets/*.* as the extra files.

domenic · 2022-11-23T07:02:38Z

assets/model-diagram.svg

@@ -0,0 +1,50 @@
+<svg xmlns="http://www.w3.org/2000/svg" aria-labelledby="chart-title-mermaid-1669067354847 chart-desc-mermaid-1669067354847" style="max-width:396.227px;background-color:#fff;font-family:&quot;trebuchet ms&quot;,verdana,arial,sans-serif;font-size:16px;fill:#333" viewBox="0 0 396.227 815">


This image is unreadably small when viewed standalone. https://raw.githubusercontent.com/whatwg/storage/651b26d44e2572123dbc82f93adb61359ed71dda/assets/model-diagram.svg

Will it be extremely small when embedded in the spec, too?

I'm not quite sure what's happening, but on Firefox it seems it gets smaller with larger viewport and gets bigger with smaller-but-bigger-than-the-image viewport. Anyway we set width and height manually in HTML, so it should be fine.

domenic · 2022-11-23T07:03:18Z

assets/model-diagram.svg

+  <path fill="none" stroke="gray" marker-end="url(#a)" marker-start="url(#b)" d="M187.818 445v100" class="er relationshipLine"/>
+  <path fill="none" stroke="gray" marker-end="url(#d)" marker-start="url(#b)" d="M187.818 620v100" class="er relationshipLine"/>
+  <path fill="#f0fff0" fill-opacity="100%" stroke="gray" d="M20 20h107.523v75H20z" class="er entityBox"/>
+  <text class="er entityLabel" dominant-baseline="middle" style="font-family:&quot;trebuchet ms&quot;,verdana,arial,sans-serif;font-size:16px" text-anchor="middle" transform="translate(73.762 57.5)">User agent</text>


I wonder why these style attributes exist, despite the overall SVG element already having that style. Oh well; I assume there's nothing we can do.

I just looked at this and SVGOMG has a "Style to attributes" option that indeed reduces the file size further. Not sure how to use that from CLI however.

The other things I see are the redundant aria-* attribute and a bunch of useless class attributes.

Corresponds to whatwg/storage#152.

domenic

Looking promising!

Makefile

domenic · 2022-11-23T12:42:52Z

scripts/model-diagram.mmd

@@ -0,0 +1,12 @@
+erDiagram


Hmm, now this lives in scripts/, which is a bit weird... maybe it should live in top-level?

Unsure how it would look if we get more diagrams... But for now SGTM.

Co-authored-by: Domenic Denicola <[email protected]>

annevk · 2022-11-30T15:51:10Z

@domenic any final feedback from you here? I have a couple minor things I can do in a fixup, but I want to make sure we're aligned on the big picture stuff.

domenic · 2022-12-01T08:16:03Z

storage.bs

+<p><img src=assets/model-diagram.svg alt="Storage model diagram (described in the next paragraph)."
+width=434 height=815>
+
+<p id=model-description>To isolate this data this standard defines a <a for=/>storage shed</a>


I guess this id="" is not really needed anymore. (Although it doesn't hurt.)

annevk

I fixed my nits, but I realized I have another question for @saschanaz.

model-diagram.mmd

Co-authored-by: Anne van Kesteren <[email protected]>

Corresponds to whatwg/storage#152.

Add a diagram for storage model

44ef0bd

saschanaz requested a review from annevk November 21, 2022 22:20

Add alt

f8ca505

annevk reviewed Nov 22, 2022

View reviewed changes

saschanaz added 2 commits November 22, 2022 13:03

Use package.json

5a5937c

Update comment

0dcada7

annevk reviewed Nov 22, 2022

View reviewed changes

Update storage.bs

651b26d

Co-authored-by: Anne van Kesteren <[email protected]>

domenic reviewed Nov 23, 2022

View reviewed changes

Apply feedback, needs spec-factory change first

0b0ac38

saschanaz added a commit to saschanaz/spec-factory that referenced this pull request Nov 23, 2022

Add whatwg/storage configs

f36dfbf

Corresponds to whatwg/storage#152.

saschanaz mentioned this pull request Nov 23, 2022

Add whatwg/storage configs whatwg/spec-factory#44

Merged

Checkin lockfile?

b3ee1e9

domenic reviewed Nov 23, 2022

View reviewed changes

saschanaz and others added 5 commits November 23, 2022 13:55

Update Makefile

2457cda

Co-authored-by: Domenic Denicola <[email protected]>

move to top level

e648f96

We are blocked; for now let's just checkin the diagram

52dcf2b

revert build.yml

5a29657

gitignore package-lock for now

83b1dd1

domenic approved these changes Dec 1, 2022

View reviewed changes

nits

2eccb79

annevk reviewed Dec 1, 2022

View reviewed changes

model-diagram.mmd Outdated Show resolved Hide resolved

Update model-diagram.mmd

ac84d5f

Co-authored-by: Anne van Kesteren <[email protected]>

annevk merged commit 8b93c4c into whatwg:main Dec 1, 2022

annevk pushed a commit to whatwg/spec-factory that referenced this pull request Dec 1, 2022

Update for Storage

0a9fa71

Corresponds to whatwg/storage#152.

saschanaz deleted the model-diagram branch December 1, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editorial: Add a diagram for storage model #152

Editorial: Add a diagram for storage model #152

saschanaz commented Nov 21, 2022 •

edited by pr-preview bot

Loading

saschanaz commented Nov 21, 2022 •

edited

Loading

aardrian commented Nov 22, 2022

domenic commented Nov 22, 2022 •

edited

Loading

saschanaz commented Nov 22, 2022 •

edited

Loading

aardrian commented Nov 22, 2022

annevk Nov 22, 2022

saschanaz Nov 22, 2022

annevk left a comment •

edited

Loading

annevk Nov 22, 2022

saschanaz Nov 22, 2022

domenic Nov 23, 2022

domenic Nov 23, 2022

domenic Nov 23, 2022

saschanaz Nov 23, 2022

saschanaz Nov 23, 2022

domenic Nov 23, 2022

domenic Nov 23, 2022

saschanaz Nov 23, 2022

domenic Nov 23, 2022

domenic Nov 23, 2022

saschanaz Nov 23, 2022

domenic Nov 23, 2022

annevk Dec 1, 2022

annevk Dec 1, 2022

domenic left a comment

domenic Nov 23, 2022

saschanaz Nov 23, 2022

annevk commented Nov 30, 2022

domenic Dec 1, 2022

annevk left a comment

		@@ -0,0 +1,50 @@
		<svg xmlns="http://www.w3.org/2000/svg" aria-labelledby="chart-title-mermaid-1669067354847 chart-desc-mermaid-1669067354847" style="max-width:396.227px;background-color:#fff;font-family:"trebuchet ms",verdana,arial,sans-serif;font-size:16px;fill:#333" viewBox="0 0 396.227 815">

Editorial: Add a diagram for storage model #152

Editorial: Add a diagram for storage model #152

Conversation

saschanaz commented Nov 21, 2022 • edited by pr-preview bot Loading

saschanaz commented Nov 21, 2022 • edited Loading

aardrian commented Nov 22, 2022

domenic commented Nov 22, 2022 • edited Loading

saschanaz commented Nov 22, 2022 • edited Loading

aardrian commented Nov 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Nov 30, 2022

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

saschanaz commented Nov 21, 2022 •

edited by pr-preview bot

Loading

saschanaz commented Nov 21, 2022 •

edited

Loading

domenic commented Nov 22, 2022 •

edited

Loading

saschanaz commented Nov 22, 2022 •

edited

Loading

annevk left a comment •

edited

Loading