Improve Text Label Scaling #230
Conversation
naglepuff
left a comment
naglepuff
left a comment
There was a problem hiding this comment.
Overall this looks good, and the text layers are all much easier to read.
One thing that might still be a concern is how to time labels bunch up on the x-axis:
Do you think it would be an improvement to change the font size (i.e. make it smaller) for lower x-scale values?
| geoViewer.value.zoomRange({ | ||
| // do not set a min limit so that bounds clamping determines min | ||
| min: -Infinity, | ||
| // 4x zoom max |
There was a problem hiding this comment.
Is this comment still true now that the value of max has changed?
There was a problem hiding this comment.
I was playing around with it because even a 2x scaling mode on a image that is 64k pixel wide is crazy, I'll revert it.
| onZoom(event: {zoomLevel: number}) { | ||
| this.zoomLevel = event.zoomLevel; | ||
| this.textScaled = undefined; | ||
| if ((this.zoomLevel || 0) < -1.5 ) { | ||
| this.textScaled = -1.5; | ||
| } else if ((this.zoomLevel || 0) > 0) { | ||
| this.textScaled = Math.sqrt(this.zoomLevel || 1); | ||
| } else { | ||
| this.textScaled = this.zoomLevel; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's worth noting that this function is the same as the one on freqLayer, and could potentially be moved to a parent class or interface. not saying it needs to be done here, but as these layer classes get more and more of the same functions, I think the argument for a parent class gets stronger.
There was a problem hiding this comment.
It was something I was debating doing. It really only applies to layers that have Text values in them.
That would be
freqLayer, legendLayer, speciesLayer, timeLayer.
so maybe there is a secondary Abstract Base layer type that is a TextLayer that will have this function and items.
There was a problem hiding this comment.
You don't have to do it here, I could see an argument for it being out of scope. As long as its tracked as an issue or a code comment at the least.
|
Created the Abstract BaseTextLayer that abstracts out duplicated properties and functions into a base class. I attempted to make a font-scaling tool for when the user adjusts the xScale (thanks I missed that in my initial testing). It's not perfect because if you reduce the compressed scale to 1 and then zoom in, the text remains fairly small. It may need some more logic to handle that conditional. |
naglepuff
left a comment
naglepuff
left a comment
There was a problem hiding this comment.
LGTM! I left one comment about code style. Feel free to merge after fixing that
| this.zoomLevel = event.zoomLevel; | ||
| this.textScaled = undefined; | ||
| if ((this.zoomLevel || 0) < -1.5 ) { | ||
| this.textScaled = -1.5; | ||
| } else if ((this.zoomLevel || 0) > 0) { | ||
| this.textScaled = Math.sqrt(this.zoomLevel || 1); | ||
| } else { | ||
| this.textScaled = this.zoomLevel; | ||
| } | ||
| this.redraw(); | ||
| } |
There was a problem hiding this comment.
Looks like this function just needs to be indented.
2 participants
I believe this was part of feedback related to the UI.
Previous Problem:
Updates:
BATAI-TextScaling_small.mp4