Feature/1280 print scale bar update #1293
Conversation
|
Hang on with the reviews. We've had some additional comments from others regarding this. |
Just let me know when you are ready! |
| @@ -45,6 +47,15 @@ class Print extends React.PureComponent { | |||
| props.options.scales = this.scales; | |||
| } | |||
|
|
|||
| // Prepare scaleMeters from admin options, fallback to default if needed | |||
There was a problem hiding this comment.
I would rather see the use of indexOf(",") > 0 to prevent double split, but its not just in this new code so .... ignore my comment.
| scaleBarLengths = { | ||
| 100: 2.5, | ||
| 200: 5, | ||
| defaultScaleBarLengths = { |
There was a problem hiding this comment.
Please explain in a comment here, why the default values has these values. To make it look good?
There was a problem hiding this comment.
I thought these values where need for other kommuner, maybe the use more scales than Varberg, for instance. But if not I can remove the unnecessary ones.
| const scaleBarFirstDigits = parseInt( | ||
| scaleBarLengthMetersStr.substring(0, 2) | ||
| ); | ||
| const startsWithDoubleDigits = |
There was a problem hiding this comment.
Is it possible to explain this in a little comment?
| divNrString = divNr.toLocaleString(); | ||
|
|
||
| // We need to make sure correct placement if dividerString is a fraction number | ||
| const dividerStrLength = divNrString.includes(",") |
There was a problem hiding this comment.
We can't use includes(",") in this context as toLocaleString() is used in the lines above.
It will work in Sweden but not in US.
|
Ready for a second opinion. |
There was a problem hiding this comment.
Looks really good! 🥳 One thing i found was that the divider number could change depending on the chosen DPI, see images below:
1:100 000 72 DPI
1:100 000 150 DPI
1:100 000 300 DPI
The behavior above is OK for me, but maybe someone else sees issues with that?
Also, remember to sync with develop before merge :)
|
It took me a while to figure out @Hallbergs comment, but now I see and agree. (If you're having trouble like me spotting it, here's a hint: take a look at the numbers printed on the scale bar in the second, 150 DPI, picture.) I'm fine with it too but if it's an easy fix, it's welcome. |
|
The narrower dividers on the first half of the scale bar are really nice! Great work! |
|
I missed a warning when reviewing... When you're syncing with develop, make sure to fix this one as well. |
So strange. This actually does not happen in our dev environment at 90 or 150dpi. :/ |
|
Thanks for the nice feedback. Going to fix the number bug and the divider :) |
|
I think this will fix the problem:
I think the Math.floor sometimes returned 4 instead of 5 when the divLinesArray.length was 10. The new code makes sure that if divLinesArr is equally divisible by 2, midIndex gets set to half of that value. |
closes #1280