feat(coremedia-richtext): TSdoc and allowEmpty (XLink) #201
Conversation
This file contains 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
Adds TSdoc and a new feature to `setXLinkAttributes` in `XLink` tooling. `allowEmpty` is required to be `true` if, for example, you need to explicitly set a value for a required attribute and it must not be skipped (such as an `<a>` which aways requires `xlink:href` to comply with CoreMedia Rich Text DTD 1.0. To not break existing usages, `allowEmpty` must default to `false`.
…inkAttributes` As `setXLinkAttributes` and `setXLinkDataSetEntries` are kind of meant to "work as a pair", both should provide a comparable experience. * Added `allowEmpty` as just done for `setXLinkAttributes`. * Added TSdoc for the method Note, that this is a breaking change, as, different from which the line-comment suggested, this method did not ignore empty values. Now it does (by default). All usages should be reviewed and possibly adapted. BREAKING CHANGE: `setXLinkDataSetEntries` now by default ignores entries with empty values.
The system test _Should correctly render broken image with empty src_ fails. This is (most likely) due to the breaking API change of `setXLinkDataSetEntries`. To grant the old behavior, adding `allowEmpty = true` to the call.
In many use-cases it makes more sense not to ignore empty values for XLink entries. Such as for attributes that are required (`xlink:href` for `<a>` and `<img>`). This reverts the previous decision to "break" the API of `setXLinkDataSetEntries`. Instead, the breaking change now applies to `setXLinkAttributes`. Still, the goal is, to keep both aligned. `allowEmpty = true` as default is also considered "more harmless" as the result is less surprising, if you invoke this API. If empty values are ignored, you may be surprised, if you did not read the TSdoc (or code) before. BREAKING CHANGE: `setXLinkAttributes` may need to set `allowEmpty = true` explicitly. Reverting previous breaking change for `setXLinkDataSetEntries`.
Now that `allowEmpty` defaults to `true`, there is no need anymore to adapt `ImageElements.ts`.
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.
Adds TSdoc and a new feature to
setXLinkAttributesinXLinktooling.allowEmptyis required to betrueif, for example, you need to explicitly set a value for a required attribute and it must not be skipped (such as an<a>which aways requiresxlink:hrefto comply with CoreMedia Rich Text DTD 1.0.To not break existing usages,
allowEmptymust default tofalse.Decide on breaking change:
As additional part of this PR, also decided to align the behavior of
setXLinkDataSetEntrieswith
setXLinkAttributes, thus, to (by default) drop attributes with empty values. While thisis aligned with the DevNote that existed before, this has to be considered a breaking change.
Usages that rely on "empty values" to be accepted, need to explicitly add
allowEmpty = trueto the call.The defensive option would be to set the default to
truefor this method, but this again makes it harder to "learn the API by heart" due to the difference ofsetXLinkAttributes.As an alternative to this, perhaps even the better one, is to make
allowEmptydefault totruefor both. For a newly developed API, this would be my recommendation as "stripping given (empty) values" is more of a convenience behavior that spares some extra lines to filter the attributes before handing them over to the API.This again, would of course also be breaking, but this time for
setXLinkAttributes.