Simplify setSubscript, setSuperscript #4190
Conversation
|
@denim2x this is how i prefer the code to be. If you want to discuss about why/ho or if you see something wrong in this approach, or you have strong opinions on the good in your version, i m very open to talk about it. |
asturur
changed the title
| var fontSize = this.getValueOfPropertyAt(line, char, 'fontSize'), | ||
| baseline = this.getValueOfPropertyAt(line, char, 'deltaY') + fontSize * schema.baseline; | ||
| this.setPropertyAt(line, char, 'deltaY', baseline); | ||
| this.setPropertyAt(line, char, 'fontSize', fontSize * schema.size); |
src/shapes/text.class.js
Outdated
|
|
||
| _setScript: function(line, char, schema) { | ||
| var fontSize = this.getValueOfPropertyAt(line, char, 'fontSize'), | ||
| baseline = this.getValueOfPropertyAt(line, char, 'deltaY') + fontSize * schema.baseline; |
There was a problem hiding this comment.
that's supposed to be ... + fontSize * -schema.baseline (follows the baseline-shift attribute from SVG)
src/shapes/text.class.js
Outdated
| return this._setScript(line, char, this.subscript); | ||
| }, | ||
|
|
||
| _setScript: function(line, char, schema) { |
|
... the rest looks ok |
|
after swapping the signs we should either rename |
|
Names are fine. We do not mirror any name other that for comfort. Fabric will not have support for baseline has people knows it, |
|
fine |
There are some reasons behind having the code in this simpler form:
providing that we are not duplicating code or violating any normal coding good practice, this code is smaller.
The functionality behind
*fontSizewas a singular case that would have need its own doc, explanationall the fabric functions for style set char by char, introducing a
many lines, many charswould have make the other methods impair. Also we have setSelectionStyles to do this and can be used in the same way.i believe that for who comes tomorrow this function _setScript is very easy to read.