Annotation size & text alignment #1551
Conversation
This reverts commit efe8345.
| width: { | ||
| valType: 'number', | ||
| min: 0, | ||
| dflt: 0, |
There was a problem hiding this comment.
Obviously the default value isn't 0, it's computed from the input text. I believe in plotly.js lingo that means the dflt value should be set to null (similar to e.g. contours.start and xbins.start)
There was a problem hiding this comment.
good call -> e01a765
| }, | ||
| valign: { | ||
| valType: 'enumerated', | ||
| values: ['top', 'middle', 'bottom'], |
| .classed('annclip', true) | ||
| .attr('id', annClipID) | ||
| .append('rect'); | ||
| annTextClip.exit().remove(); |
There was a problem hiding this comment.
can we add a jasmine test, checking that the clip paths are removed when removing an annotation?
There was a problem hiding this comment.
In fact they're not removed - they don't proliferate, ie if you repeatedly make 100 annotations and delete them, you'll never get more than 100 clip paths, but I didn't bother deleting clip paths corresponding to annotations past the end of the new array after removing some. I can revisit that though...
There was a problem hiding this comment.
Ha I see. We can revisit this when we shift to a more d3-idiomatic pattern in annotation draw. Then, we could easily remove those <clipPath> inside the exit() selection similar to how updatemenu buttons are removed.
There was a problem hiding this comment.
actually was easy to remove the unneeded clip paths after all -> b21154b
Hmm. I guess what we really need is some auto-textwrapping routine. I can expect some users not liking having to add |
| coerce('width'); | ||
| coerce('height'); | ||
| coerce('align'); | ||
| coerce('valign'); |
There was a problem hiding this comment.
I guess we don't need to coerce valign is height isn't set?
At some point that'd be a nice feature, but not now, it's going to be annoying. For now I just put a note about this in the description of width: |
Nice. Good enough for now! |
| if(!optionsIn || options.visible === false) return; | ||
| if(!optionsIn || options.visible === false) { | ||
| d3.selectAll('#' + annClipID).remove(); | ||
| return; |
|
Nicely done 💃 This will be part of |
Explicit width and height for annotations, along with vertical alignment to complement the existing (horizontal) alignment.
@etpinard a couple of decisions could use 👍 /👎 :
valign, and its default value of'middle'- we already have defaultalign: 'center', so now if you make a giant annotation the text will end up in the center (as in the bottom left annotation I added to https://github.com/plotly/plotly.js/compare/note-sizing?expand=1#diff-038299cfa4b6b29a6fad48dc78a8c1d0)borderpadregion is always blank - see the bottom center annotation added in that same baseline image. You could argue for allowing it all the way to the border, but to me it looks better this way, and I think this overrides letting the user see another half a letter or whatever. It has a quirk though, that even if I'm clipping the text in principle exactly to its own bounding box, sometimes you lose a tiny bit. I mostly fixed this problem by not clipping at all if you don't set an explicit width or height (when clipping shouldn't do anything anyway) but it might still pop up on the in-principle-not-clipped sides of clipped text. I suppose there could be a solution to this involving stretching the clip rectangle on the non-clipped sides, based on whetherwidthand/orheightare set and the values ofalignandvalignbut to me that sounds more complicated than it's worth. You can see what I'm talking about if you look at my intermediate commit efe8345 - the "j"s in justified and the "y" in y-int lost a tiny bit of the left and bottom respectively of their tails.