-
Notifications
You must be signed in to change notification settings - Fork 125
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
Remove couple of lint checks and modify another, add CSS to core to support new hgroup definition #593
Conversation
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.
Changes to core.css need to be replicated over to the test suite’s core.css as well.
@@ -45,6 +45,32 @@ hgroup{ | |||
text-align: center; | |||
} | |||
|
|||
/* simulate h3 in an hgroup */ | |||
hgroup h2 + p{ |
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.
Could we use a child selector here for performance? hgroup > h2 + p
@@ -262,7 +262,6 @@ | |||
"s-071", "Sectioning element with more than one heading element." | |||
"s-072", "Element with single [xhtml]<span>[/] child. [xhtml]<span>[/] should be removed and its attributes promoted to the parent element." | |||
"s-073", "Header element that requires [val]label[/] and [val]ordinal[/] semantic children." | |||
"s-074", "[xhtml]<hgroup>[/] element containing sequential [xhtml]<h#>[/] elements at the same heading level." |
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.
Good to list removed rules as available for the next time we want to add one.
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 didn't know the tests had a core.css; I'll add that.
I used the structure Alex initially posted. These are epubs, not high-traffic web sites, so I can't imagine that makes the slightest difference at all in performance on an ereader, but I'll be glad to make the change if Alex prefers.
List them where? They're available if they're not being used. :)
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.
Given the state of CPUs on ereaders I’d say it’s more important here than on a website, but it’s a minimal difference either way.
Have a look at this commit for an example of retiring a lint rule: 98fa396
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.
To be clear: I don't care which we use, and what you suggest is what I would have done had I written it from scratch. But I learned long ago that when Alex suggests CSS, that's the CSS to use. :)
That UNUSED block no longer exists. I'm sure there's a way in git to figure out when it went away, but I don't know what it is (without looking at every commit involving se_epub_lint, which I'm not going to do). Regardless, it's not there now. And, again, anything that is not in the used list is by definition unused, so I don't understand why it would be needed anyway.
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.
Add a new equivalent block to the bottom of the s-
rules. The block is removed when those unused rules are all reused. It’s just there as a reference so that those numbers can be reused in future rule additions. Keeps the numbering in check, and everyone’s OCD happy.
hgroup h5 + p{ | ||
font-size: .67em; | ||
} | ||
|
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.
Also, we should add an hgroup p
selector to line 38 so that the <h#>
styling carries on to <p>
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.
And, we also will need text-indent: 0
to all of these new selectors, to override p + p
text indent styling in case we have multiple <p>
in an <hgroup>
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.
Those h#/hgroup selectors (at 38) have a 3em margin, which would give each p in the hgroup that margin (I tested to confirm). I'm assuming we don't want that. :) Would you prefer to make a separate selector for just hgroup p and put everything but the margin on that, or add hgroup p to the existing group, remove margin, and make a separate selector for everything but hgroup p with just the margin on it?
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.
We do want that, we want to replicate <h#>
styling for these new <p>
s. <h#>
has a 3em margin so the <p>
s should also. Margins of direct siblings will collapse.
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.
If we make a separate hgroup p
selector and put everything in it but the margin, we can also add the text-indent: 0
, so we don't have to add it to each of the other selectors. We also don't need the text-align: center
, since that will be handled by the hgroup
selector in the original. So:
hgroup p{
break-after: avoid;
break-inside: avoid;
font-variant: small-caps;
hyphens: none;
-epub-hyphens: none;
text-indent: 0;
}
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.
Hyphens will inherit. It's useful to open it in a browser and use the browser's CSS inspector to see how things are being inherited and what's overriding what.
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.
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 see, I made a mistake, though for a different reason. Line 48 removes margins for all children of <hgroup>
. So the margins aren't important after all.
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.
All right, I updated the message and added the unused block, and did some more testing with the CSS.
I created a test chapter with an hgroup with h2 thru h4 elements. With the current core.css, or the new core.css, they display like this.
If I change them to p's, however, and add hgroup p
to the selector in question, they display like this:
Which is what I would expect. As h#s, they are caught by the hgroup > *
selector, which zeroes their margin. We have to have that since a standalone h# should have a margin, but one in an hgroup shouldn't. However, with the addition of hgroup p
to the other selectors, that has higher precedence, and so the p's in the hgroup are caught by it rather than by hgroup > *
, and we get margins inside the hgroup, which we don't want. Thus, we don't need the hgroup p
; the hgroup
handles the hgroup margins, and we don't want margins within the hgroup.
There is another complication however. There is a separate set of selectors (starting with p.continued,
) that ensure the first paragraph after an h# (and hr, etc.) aren't indented, and use hanging punctuation. Several of the selectors are of the form h# + p
, which of course now matches the p's in the hgroup, and so they are handled by that selector. The good news is that gives us the text-indent: 0
we wanted; the other news is that that also gives them hanging punctuation, which maybe we want in the header and maybe we don't, and maybe it doesn't matter since they're centered?
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.
Sorry, I didn't see your followup message until just now. So the only new news is the above complication.
if nodes: | ||
messages.append(LintMessage("s-070", "[xhtml]<h#>[/] element without [xhtml]<hgroup>[/] parent and without semantic inflection.", se.MESSAGE_TYPE_WARNING, filename, [node.to_string() for node in nodes])) | ||
messages.append(LintMessage("s-070", "[xhtml]<h#>[/] element without semantic inflection.", se.MESSAGE_TYPE_WARNING, filename, [node.to_string() for node in nodes])) |
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.
Please make sure to change the message to match in the big list of messages at the top of the file too
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.
Doh! I thought of that once and obviously didn't get back to change it. I have RL things today, I'll make this and the rest of the changes tonight after I hear from you on the above.
0042760
to
ab9470e
Compare
… as of epubcheck 5.1)
… elements in an hgroup
ab9470e
to
10c407d
Compare
OK I think this looks good, thanks! |
No description provided.