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

Make page-spread-center an alias for spread-none #1950

Merged
merged 10 commits into from
Jan 19, 2022
Merged

Make page-spread-center an alias for spread-none #1950

merged 10 commits into from
Jan 19, 2022

Conversation

mattgarrish
Copy link
Member

@mattgarrish mattgarrish commented Dec 1, 2021

The only concern I have now that I'm opening the PR is whether we should formally deprecate page-spread-center or if we should "discourage" its use. I don't know if anyone has used it that we'd affect by this change.

Otherwise, the PR itself removes all traces of page-spread-center except to deprecate and make it an alias and adds the centering prose to the rendition:spread none processing requirements.


Preview | Diff

epub33/rs/index.html Outdated Show resolved Hide resolved
Copy link

@shiestyle shiestyle left a comment

Choose a reason for hiding this comment

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

I understand this change but most EPUB 3 files in Japan use 'page-spread-center' for 1st page as cover and we should avoid that EPUBCheck will recognize it as error and warning.

So alias is acceptable but deprecated is not good label.

@mattgarrish
Copy link
Member Author

most EPUB 3 files in Japan use 'page-spread-center' for 1st page as cover

That's what I was worried about. I'm perfectly fine with only making it an alias and not deprecating.

clarify reason for the property in the note on aliases;
additional minor cleanup of the page-spread-* section
# Conflicts:
#	epub33/core/index.html
#	epub33/rs/index.html
…to fix/issue-1929

# Conflicts:
#	epub33/core/index.html
#	epub33/rs/index.html
# Conflicts:
#	epub33/core/index.html
#	epub33/rs/index.html
@mattgarrish
Copy link
Member Author

mattgarrish commented Dec 8, 2021

Last few commits make the following changes:

  • removes the deprecated label from page-spread-center
  • adds an explanation of what the property does to its definition - I also moved up the explanations of -left and -right into their definitions as it was odd to only say they are aliases and then in a following paragraph explain what they do
  • revises the note about the aliases to remove the repeated statement of what -left and -right are aliases for. I also added a new paragraph explaining why -center exists as an alias (to simplify thinking about two-page and single layouts, but check this makes sense)
  • restores the example of using page-spread-center but without the confusing text about not needing to apply spread-none since its role as an alias is now clear

@wareid wareid linked an issue Dec 8, 2021 that may be closed by this pull request
Copy link

@shiestyle shiestyle left a comment

Choose a reason for hiding this comment

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

Thanks, Matt. That's fine for me.

@iherman
Copy link
Member

iherman commented Dec 10, 2021

The issue was discussed in a meeting on 2021-12-09

  • no resolutions were taken
View the transcript

2. Is page-spread-center a synthetic spread or not? (issue epub-specs#1929)

See github issue epub-specs#1929.

See github pull request epub-specs#1950.

Wendy Reid: i just got through writing the tests for all the FXL rendition properties, which lead to a question about page-spread-center.
… this is a spine property that is not meant to override spread behaviour, but that the page should be centered in the viewport.
… but this doesn't seem to be what is happening in practice.
… rather, the observed behaviour seems to be identical to spread none.
… we've discussed in the issue, and mgarrish came up with the idea of making page-spread-center an alias of spread none.
… and not to deprecate page-spread-center.

Brady Duga: not sure why page-spread-center is confusing. Imagine you are doing a synthetic spread, but you want one specific page centered in the middle of the screen. That's how you would tell the RS to do that..
… spread none sounds like you've turned off spreads completely.

Wendy Reid: an additional level of oddness was finding out that you can put page spread properties on spine level items as well.

Brady Duga: spread none and spread center mean different things, because spread none doesn't tell you where to put the page.
… and yes, it is confusing that you can put this on a spine item (you can also alternate portrait and landscape on spine level items as another weird way of interacting with spreads).

Dave Cramer: is there a concern that this could affect the relative size of pages as you go from spread, to non-spread, and back to spread?.

Brady Duga: i can't see people really doing this.
… one potential use case is if one of your content documents is already a spread, so you want to disable synthetic spreads for that one only.

Shinya Takami (高見真也): in Japan we use spread-center for the cover pages in many many cases, so making spread-center a depreciated feature would not be acceptable.
… aliasing it to spread none is okay.

Wendy Reid: right, so we won't deprecate page-spread-center.
… this might be the sort of thing we need to test out.
… when the page appears in the center of the viewport, i think there is a change in the page size.

Dave Cramer: i'm not comfortable deciding this until we have more info on whether we need to better define how a RS should position the viewport when you use page-spread-center or spread none as an override of a synthetic spread epub.

Wendy Reid: agree that the FXL properties section could use some visual aides.
… we can put this off for tonight.

Brady Duga: if we end up keeping page spread then we need to fix this definition.

Wendy Reid: yeah, definitions in that section are not very good.

Dave Cramer: it was a long time ago that we wrote those, we were balancing against implementations that already existed at the time (amazon, apple).

@mattgarrish mattgarrish changed the title Deprecate page-spread-center and make it an alias for spread-none Make page-spread-center and make it an alias for spread-none Dec 10, 2021
@mattgarrish mattgarrish changed the title Make page-spread-center and make it an alias for spread-none Make page-spread-center an alias for spread-none Dec 10, 2021
@iherman iherman merged commit 6cf4408 into main Jan 19, 2022
@iherman iherman deleted the fix/issue-1929 branch January 19, 2022 13:28
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.

Is page-spread-center a synthetic spread or not?
5 participants