-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor terms and definitions sections #284
Conversation
@ProgramMax Can you see if this is going in the right direction? |
Oh my, this pull request got very big. No matter. I'll take a look. But I might not finish tonight. |
I tried to separate them as commits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew.
This pull request has a lot of really good changes.
Thank you for doing all this work. This was clearly a hassle.
I have a few comments/changes.
index.html
Outdated
</dt> | ||
<dd>form an image by merging a foreground image and a background image, using transparency information to determine | ||
where and to what extent the background should be visible | ||
<div class="note">The foreground image is said to be <a>composited</a> against the background.</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier, under 'Chromaticity', a <p>
was used for the note. Should that be used here instead of a <div>
?
index.html
Outdated
|
||
<dt><dfn>transfer function</dfn> | ||
</dt> | ||
<dd>image where reference black and white correspond, respectively. to sample values <code>0</code> and <code>2<sup>bit depth</sup> - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably didn't mean to have a period after "respectively"
different sample depths. | ||
</dd> | ||
<!-- Maintain a fragment named "3sample" to preserve incoming links to it --> | ||
<!-- need a definition of frame --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need filled in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://w3c.github.io/PNG-spec/#apng-frame-based-animation should probably contain that definition. File a separate issue?
index.html
Outdated
|
||
<p>Some PNG images — called <dfn class="lint-ignore">Animated PNG</dfn> (<abbr title="Animated PNG">APNG</abbr>) — also | ||
contain a frame-based animation sequence, the <dfn>animated image</dfn>. The first frame of this may be — but need not be — | ||
the <a>static image</a>. Mon animation-capable displays (such as printers) will display the <a>static image</a> rather than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-"Mon" +"Non-"
index.html
Outdated
from black to white). The alpha channel may be represented by a single <a>grey sample</a> value: matching pixels are fully | ||
transparent, and all others are fully opaque. If the alpha channel is not represented in this way, all pixels are fully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wording is unclear to me.
It sounds like you're saying if the gray scale pixel has the value 86 and the alpha channel also has 86, this pixel will be transparent. Otherwise, this pixel is fully opaque.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if this works better. If so, we should probably change the similar sentence under Truecolour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that wording is better, yeah. Thank you :)
And good catch on Truecolour needing an update, too.
index.html
Outdated
<h3>Output buffer</h3> | ||
|
||
<p>The <dfn>output buffer</dfn> is a pixel array with dimensions specified by the width and height parameters of the PNG <a | ||
class= "chunk" href="#11IHDR">IHDR</a> chunk. Conceptually, each frame is constructed in the output buffer before being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the space between class= and "chunk" matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review. I plan to let this sit until our next call. Ok? Should we file tickets re: truecolor+alpha and definition of frame? |
Waiting is fine by me. I filed two issues as you suggested. |
@ProgramMax I am ok to merge now if you think it is ok as well — the changes are supposed to be editorial by nature. |
I think it is okay. I generally let the author merge just in case they suddenly go "Oh wait, I meant to add a thing!" |
Closes #156