ui-speedspacechart: speed limit tags enhancements #560
Conversation
Caracol3
added
kind:bug
- adding logic to make the tooltip follow the cursor move horizontally and vertically - sticking the tooltip to the chart when the cursor goes outside - adding logic to prevent text going outside its tag Signed-off-by: Mathieu <[email protected]>
Caracol3
force-pushed
the
mcy/speedspachechart-speed-limit-tags-linear-layer-enhancements
branch
from
2255b77
to
960c78a
Compare
| const TEXT_LEFT_PADDING = 4; | ||
| const TEXT_RIGHT_PADDING = 8; | ||
| const FIRST_TAG_LEFT_PADDING = 8; | ||
| const TEXT_RIGHT_PADDING = 4; |
There was a problem hiding this comment.
you can merge:
TEXT_LEFT_PADDINGandTEXT_RIGHT_PADDINGtoTEXT_PADDINGICON_WIDTHandICON_RIGHTtoICON_SIZEICON_BACKGROUND_WIDTHandICON_BACKGROUND_HEIGHTtoICON_BACKGROUND_SIZE
There was a problem hiding this comment.
Yes, we talked about it but if i think it will be a problem with the logic then.
like here:
const textRectWidth = Math.min(
actualTextWidth + TEXT_LEFT_PADDING + TEXT_RIGHT_PADDING,
tagWidth
);There was a problem hiding this comment.
const textRectWidth = Math.min(
actualTextWidth + TEXT_PADDING * 2,
tagWidth
);There was a problem hiding this comment.
If it's more readable, no problem to change this. I thought it was clearer to understand by adding the two paddings, but indeed, apart from left and right, we don't have other options, so go for '*2'.
| @@ -160,18 +136,62 @@ export const drawSpeedLimitTags = async ({ | |||
| ) { | |||
| tooltip = { | |||
| cursorX: cursor.x + MARGIN_LEFT, | |||
| cursorY: Y_POSITION, | |||
| cursorY: cursor.y, | |||
| text: 'Incompatible with the infrastructure', | |||
There was a problem hiding this comment.
| text: 'Incompatible with the infrastructure', | |
| text: 'Incompatible with the infrastructure', |
we should begin to add those texts in the user translations object prop, then keep this trad as a placeholder
| tagWidth | ||
| ); | ||
|
|
||
| ctx.save(); |
There was a problem hiding this comment.
| ctx.save(); |
seems useless
| textRectWidth - TEXT_LEFT_PADDING | ||
| ); | ||
|
|
||
| ctx.restore(); |
There was a problem hiding this comment.
| ctx.restore(); |
seems useless
|
|
||
| ctx.strokeRect(x, Y_POSITION, tagWidth, RECT_HEIGHT); | ||
|
|
||
| if (tag === 'UU') { |
There was a problem hiding this comment.
you don't use it anymore, is it still usefull?
| tag_name: 'UU', | ||
| color: '#D91C1C', | ||
| tag_name: 'MA100', | ||
| color: '#494641', |
There was a problem hiding this comment.
Is it intentional to replace the UU tag with MA100 here? I noticed that you check the UU tag on line 188 in SpeedLimitTags, and it duplicates the MA100 tag at the same time.
| const constrainTooltipPosition = (cursorX: number, width: number, tooltipWidth: number) => { | ||
| if (cursorX - tooltipWidth / 2 < LEFT_MARGIN) { | ||
| return LEFT_MARGIN + tooltipWidth / 2; | ||
| } | ||
| if (cursorX + tooltipWidth / 2 > width - RIGHT_MARGIN) { | ||
| return width - RIGHT_MARGIN - tooltipWidth / 2; | ||
| } | ||
| return cursorX; | ||
| }; |
There was a problem hiding this comment.
Could you please move this function to utils? It would also be great if you could add a unit test.
|
there is a strange white border next to the |
Signed-off-by: Mathieu [email protected]