readium / readium-css

🌈 A set of reference stylesheets for EPUB Reading Systems, starting with Readium Mobile
https://readium.org/readium-css/
BSD 3-Clause "New" or "Revised" License
90 stars 20 forks source link

List of inconsistencies #21

Closed JayPanoz closed 6 years ago

JayPanoz commented 6 years ago

This issue mainly impact variables but can impact the global design (and docs) as well.

There are some inconsistencies here and there and I’d really like to correct them so that implementers don’t have to deal with some cognitive overhead: something that is not logic or natural, etc. For studying linguistics, I know how important it is to have a coherent system in which things that achieve the same look the same so do not hesitate to report issues related to this.

As far as I can tell:

JayPanoz commented 6 years ago

Variables related to background- and color (including ::selection and links) should use the same prefix: it is not the case yet (we have --RS__ and --THEME__; the most natural option would be managing that as a ReadiumCSS-day_theme.css stylesheet and use --THEME__.

This won’t be a breaking change in ReadiumCSS but it’s kinda hard to tell for implementations happening outside of EDRLab. In any case, if you’re using --RS__backgroundColor, --RS__textColor, --RS__linkColor or --RS__visitedColor in code, please let me know so that I can emphasize the change in docs.

Another issue might be using the --THEME__ prefix in the first place, as we’re talking about modes there (day, night, sepia), and themes are another thing entirely. After careful consideration, I feel like we’d be better off with a --RS__ prefix, especially as there already are --USER__ variables for this in advanced settings.

JayPanoz commented 6 years ago

FYI I’m planning to commit the first inconsistency (themes) next week. It should not impact current implementations as it is just an architectural change and the CSS variables are not set using JavaScript but CSS.

In hindsight, I’m not so sure about the third one (--USER__advancedSettings flag). I’ll need feedback from people having already implemented it. To some extent, it might be better to keep it, as it requires some extra care to implement but that’s debatable.

JayPanoz commented 6 years ago

Can finally close this issue as we now have a “Day Mode”, which serves as default.

To sum things up:

Those changes are scoped to CSS and shouldn’t impact app’s logic. But feel free to report if this isn’t the case and it breaks anything in test apps. Thanks in advance.