Skip to content
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

Adding tests for spread and page-spread properties #94

Merged
merged 17 commits into from
Dec 9, 2021
Merged

Conversation

wareid
Copy link
Contributor

@wareid wareid commented Nov 24, 2021

Accidental mega-test thread, this tests the properties of both rendition:spread and rendition:page-spread-*.

As a special bonus, there are also 2 tests for content where rendition:layout is applied in the spine.

EPUBs will be added to this PR after all changes have been addressed to save myself some pain :).

@iherman
Copy link
Member

iherman commented Nov 25, 2021

@wareid can you explain the "accidental"? Put it another way, what parts are to be safely ignored in a review? Otherwise it is difficult to review the PR...

@wareid
Copy link
Contributor Author

wareid commented Nov 25, 2021

@wareid can you explain the "accidental"? Put it another way, what parts are to be safely ignored in a review? Otherwise it is difficult to review the PR...

I got a bit carried away and did more tests in one branch than I meant to :).

tests/fxl-layout_duplication/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-layout_duplication/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_center/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_combined/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-spread_auto/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-spread_default/OPS/package.opf Outdated Show resolved Hide resolved
tests/page-layout-both/OPS/package.opf Outdated Show resolved Hide resolved
tests/page-layout-both/OPS/package.opf Show resolved Hide resolved
tests/page-layout-both_spread/OPS/package.opf Outdated Show resolved Hide resolved
@wareid
Copy link
Contributor Author

wareid commented Nov 29, 2021

I have no experience with FXL, so this question may be a throw-away. However, I tried this test on calibre, Apple Books, and Thorium, and none of these implement this properly, at least based on what I see: the colorful pages are either on the left or the right. Is it what we "expect" to see at this point, or is there a problem with the test?

Only page one should display in the centre, and the remainder should be in spreads/alone.

@wareid
Copy link
Contributor Author

wareid commented Nov 29, 2021

This anchor does not exist, I presume this should be the subject of a separate PR on the content document. However, I believe the current convention is to use the same filename of the test as an anchor (@dlazin should be the judge of this), ie, the anchor should probably be fxl-layout_duplication.

Fixing the label, but yes a PR is incoming for the specs.

@iherman
Copy link
Member

iherman commented Nov 29, 2021

I have no experience with FXL, so this question may be a throw-away. However, I tried this test on calibre, Apple Books, and Thorium, and none of these implement this properly, at least based on what I see: the colorful pages are either on the left or the right. Is it what we "expect" to see at this point, or is there a problem with the test?

Only page one should display in the centre, and the remainder should be in spreads/alone.

Right. And none of the RS-s that I tested with do that :-(

But if we are confident the test is correct, then we are fine at this point! I just wanted to be sure...

Copy link
Collaborator

@dlazin dlazin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review! More later!

tests/fxl-layout_duplication/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread_center/OPS/nav.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_center/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread_center/OPS/package.opf Outdated Show resolved Hide resolved
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-11-24</dc:date>
<dc:description>When rendition:page-spread-right and -left are applied to adjacent spine items, they should be displayed in a synthetic spread.</dc:description>
<dc:identifier id="pub-id">fxl-page-spread_combined</dc:identifier>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the spec PR, the test ID should ideally match the anchor ID. Since the anchor doesn't have "combined," can you make them match?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies.

tests/fxl-page-spread_combined/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread_left/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-spread_auto/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-spread_both/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-spread_default/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-spread_default/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-spread_landscape/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-spread_landscape/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/page-layout-both/OPS/package.opf Show resolved Hide resolved
tests/page-layout-both_spread/OPS/package.opf Outdated Show resolved Hide resolved
tests/page-layout-both/OPS/package.opf Show resolved Hide resolved
tests/page-layout-both_spread/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread_combined/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_combined/OPS/page_002.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_combined/OPS/page_003.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_combined/OPS/page_004.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_left/OPS/nav.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/page_002.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/page_003.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread_right/OPS/page_004.xhtml Outdated Show resolved Hide resolved
@wareid
Copy link
Contributor Author

wareid commented Dec 6, 2021

Think I've addressed everything, let me know if it's ok and I'll make the EPUBs and test them and merge.

tests/fxl-page-spread-center/OPS/page_002.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread-center/OPS/page_003.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread-center/OPS/page_004.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread-combined/OPS/nav.xhtml Outdated Show resolved Hide resolved
tests/fxl-page-spread-left/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread-right/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-page-spread-right/OPS/page_002.xhtml Outdated Show resolved Hide resolved
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-11-24</dc:date>
<dc:description>When rendition:page-spread-right and -left are applied to adjacent spine items, they should be displayed in a synthetic spread.</dc:description>
<dc:identifier id="pub-id">fxl-page-spread_combined</dc:identifier>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still applies.

<meta property="dcterms:modified">2021-11-24T00:00:00Z</meta>
<meta property="rendition:layout">pre-paginated</meta>
<meta property="rendition:spread">landscape</meta>
<meta property="belongs-to-collection">should</meta>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize above dc section, please.

<meta property="dcterms:isReferencedBy">https://www.w3.org/TR/epub-rs-33/#page-layout-both</meta>
<meta property="dcterms:modified">2021-11-24T00:00:00Z</meta>
<meta property="rendition:layout">pre-paginated</meta>
<meta property="belongs-to-collection">should</meta>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize above dc section, please.

wareid and others added 7 commits December 8, 2021 08:51
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
@wareid wareid merged commit 66e4295 into main Dec 9, 2021
@wareid wareid deleted the fxl-page-spread branch December 9, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants