chore: drop dead CSS declarations and deprecate no-op API - #85
Merged
Conversation
`imageCSS` declared `loading: lazy`. `loading` is an HTML attribute, not a CSS property, so every browser dropped the declaration; it only ever looked like lazy loading was enabled. Enabling it for real would mean setting the attribute on the `<img>` elements. `imageCSS` also carried percentage `min-height`/`max-height`. Both resolved against a containing block of `auto` height, so they were no-ops that contributed nothing to the responsive behaviour, and `min-height: 100%` would have stretched every image to the full container height as soon as a definite height appeared. `max-width: 100%` with `height: auto` is what does the work. `iframeCSS` used `%d` for `iframeHeight`, which is a Swift `Int` and therefore 64-bit on every platform this package supports, while `%d` reads 32 bits. It works for the current value of 250 and would silently not for a large one. `Color+Extension` already spells the same thing `%02lX`, so `%ld` matches. `bodyCSS` is referenced nowhere. Body margins are emitted directly by `cssTemplate` and `mediaCSSTemplate`, which means its `-webkit-text-size-adjust` rule has never been applied either. `Configuration.isColorsImportant` is recorded but never read during CSS generation - `!important` comes entirely from `ColorSet.isImportant` - so passing `isColorsImportant: .all` silently did nothing. Both are deprecated rather than deleted, since they are public API. Behaviour is unchanged and the colour preference still round-trips through internal storage, so the framework does not warn on its own API. A new test pins down which of the two paths actually reaches the CSS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dead code found by auditing every member of
RichTextConstantsfor real usage. None of this changes rendering.Dead CSS
loading: lazyinimageCSS.loadingis an HTML attribute, not a CSS property, so every browser dropped the declaration. It only ever looked like lazy loading was enabled; enabling it for real would mean setting the attribute on the<img>elements.Percentage
min-height/max-heightinimageCSS. Both resolved against a containing block ofautoheight, so both were no-ops that contributed nothing to the responsive behaviour - which is why nobody noticed they were broken before #76.min-height: 100%in particular would have stretched every image to the full container height the moment a definite height appeared.max-width: 100%withheight: autois what actually does the work, and is what is left.Wrong format specifier
iframeCSSused%dforiframeHeight, which is a SwiftIntand therefore 64-bit on every platform this package supports, while%dreads 32 bits. It works for the current value of 250 and would silently not for a large one.Color+Extensionalready spells the same thing%02lX, so%ldmatches the existing convention.API that does nothing
bodyCSSis referenced nowhere. Body margins are emitted directly bycssTemplateandmediaCSSTemplate, which means its-webkit-text-size-adjustrule has never been applied either.Configuration.isColorsImportantis recorded but never read during CSS generation.!importantcomes entirely fromColorSet.isImportantonfontColor/linkColor, so this silently did nothing:Both are deprecated rather than deleted, since they are public API. The colour preference still round-trips through internal storage so behaviour is unchanged and the framework does not warn on its own API.
.colorPreference(forceColor:)on the view, which sets both colour sets, is the working path and the deprecation message points at it.A new test pins down which of the two paths actually reaches the CSS: