fix(StyledText): add ability to unset style (issue #9578) #9597
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
This WORKS! I think I once the PR closes the links die, that is why it didn't work when I tried |
src/shapes/Text/StyledText.ts
Outdated
| * if not style is set for a pre determined line or char, return a new object. | ||
| * this is tricky and confusing because when you get an empty object you can't | ||
| * determine if is a reference or a new one. | ||
| * @TODO this should return always a reference or always a clone, and if necessary undefined. |
src/shapes/Textbox.ts
Outdated
| * @param {Number} lineIndex | ||
| * @param {Number} charIndex | ||
| * @private | ||
| * @return {TextStyleDeclaration} style object a REFERENCE to the existing one or a new empty object |
|
I would say that the name |
|
@ShaMan123 could you re-review this one? i would not rename the method at this point, but let me know how you feel strongly about that object code. |
src/shapes/Text/StyledText.ts
Outdated
| // then delete what is undefined in styles from newStyle | ||
| Object.keys(style).forEach((key) => { | ||
| if (style[key as keyof TextStyleDeclaration] === undefined) { | ||
| delete newStyle[key as keyof TextStyleDeclaration]; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I still don't like this
Could we use pick? filter? And have newStyle created correctly?
I am not a fan of Text and definitely not of TextStyles, so I prefer adding more straight forward code.
However, if you feel/think differently that is fine, let's move on. I have more important things to persist upon.
There was a problem hiding this comment.
so newStyle is created correctly right now, you don't like the shape of the code that creates it.
if i use filter, i have to:
- mix the 2 object together, creating a new one
- use Entries, that is a loop + creation of an array
- filter that returns the new list of entries we want to keep
- fromEntries that create a new object from the filtered list
this is more compact, but i can't call it more straight forward at all:
Object.fromEntries(Object.entries({
...this._getStyleDeclaration(lineIndex, charIndex),
...style,
}).filter(([key, value]) => value !== undefined));I can change it but you didn't mention what is unpreferable about the actual code.
The original comment was:
Since you are creating a new object I think it is better to filter out undefined beforehand instead of creating it and then deleting stuff
We are still creating a new object with all the stuff ( the spread statement ), then we are creating an array of arrays with all entries, a new one filtered, then we are creating a new object once again.
Is that you don't want to carry over an object on which has been used the delete keyword?
There was a problem hiding this comment.
With pick is overly complicated, maybe pickBy?
There was a problem hiding this comment.
pickby gives us a negative code size and probably the same speed, while the fromEntries <-> toEntries is definetely worse on both aspects
| * Get a reference, not a clone, to the style object for a given character, | ||
| * if no style is set for a line or char, return a new empty object. | ||
| * This is tricky and confusing because when you get an empty object you can't | ||
| * determine if it is a reference or a new one. | ||
| * @TODO this should always return a reference or always a clone or undefined when necessary. |
There was a problem hiding this comment.
yes we have a TODO to fix it. Fixing it now has a lot of subtle consequences i don't want to think of
asturur
changed the title
|
Addressed the comments that were added post approval |
…into set-selection-styles
Description
closes #9578
_setSelectionStyles is behaving in a very confusing way.
_getSelectionStyleis the culprit but i m not touching it right now._getSelectionStylereturns a reference to the style object or a new empty one. The reference can also be to an empty object as well. So when you get an empty object you don't really know if is an actual reference or the original one.Because of this we were checking if the returned object was empty, if it was we were anyway setting there a new empty object once again, so we would know that the text call would return a reference to the one we just created ( really strange ).
At that point we would use Object.assign to modify it.
This wasn't the bug actually, the bug is that if i pass
fontSize: undefinedi want to remove fontSize from the current style section and not merge an undefined value that breaks rendering.In Action