change(web): polish documentation + uses of transition ID in predictive text 🚂#16148
change(web): polish documentation + uses of transition ID in predictive text 🚂#16148jahorton wants to merge 2 commits into
Conversation
…ve text - Drops the `Suggestion.transformId` field in favor of just reusing `.transform.id` - Updates documentation on .transform.id as a unique identifier of the transition, not specific to a single Transform. Does not modify the `.transform` or `.id` properties because we've been publishing lexical model types, and that would change the public model API. Build-bot: skip build:web Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
Bot
changed the title
github-actions
Bot
added
web/
common/
common/web/
web/predictive-text/
change
| * Facilitates use of unique identifiers for tracking data about the context | ||
| * transition to which the Transform belongs. More than one Transform may | ||
| * hold the same `id` if they are alternate interpretations of the same | ||
| * transition event - say, the resulting effects of neighbor keys that may | ||
| * have been missed due to "fat fingering". | ||
| * | ||
| * Also note that the Transform reference cannot be preserved across WebWorker | ||
| * boundaries, but this ID may. | ||
| * | ||
| * This is *separate* from any LMLayer-internal identification values. | ||
| */ | ||
| id?: number; |
There was a problem hiding this comment.
Would this be better named transitionId if it identifies the transition the transform belongs to? Transform.id suggests that it's the identifier of the transform itself.
| * Facilitates use of unique identifiers for tracking data about the context | |
| * transition to which the Transform belongs. More than one Transform may | |
| * hold the same `id` if they are alternate interpretations of the same | |
| * transition event - say, the resulting effects of neighbor keys that may | |
| * have been missed due to "fat fingering". | |
| * | |
| * Also note that the Transform reference cannot be preserved across WebWorker | |
| * boundaries, but this ID may. | |
| * | |
| * This is *separate* from any LMLayer-internal identification values. | |
| */ | |
| transitionId?: number; |
There was a problem hiding this comment.
This is a publicly published api type for any potential Lexical Model, published via our NPM lexical-model-types package. I'd already be changing it if it weren't.
| // used to add whitespace (if one existed), so reverting to the base ID's associated | ||
| // context also reverts the appendedTransform. | ||
| let originalTransition = this.contextTracker.findAndRevert(-reversion.transformId); | ||
| const baseTransitionId = -reversion.transform.id; |
There was a problem hiding this comment.
Why do we use the negated transform.id?
There was a problem hiding this comment.
Actual transition + transform IDs are always positive. Negating it indicates association with the original ID, but "reversal" - it "negates" an effect previously placed there.
There's a pretty decent chance we don't actually need to do the negation now, now that things have advanced a bit, but it's been a pattern since 12.0.
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
LGTM; see @ermshiperete's questions. Appreciate the tightening up of the nomenclature.
IMHO, the most significant change here is the removal of Suggestion.transformId. I suggest that the title of the PR reflect that.
3 participants
Primary changes:
Suggestion.transformIdfield in favor of just reusingSuggestion.transform.idTransform.Does not modify the
.transformor.idproperties because we've been publishing lexical model types, and that would change the public model API.Build-bot: skip build:web
Test-bot: skip