-
-
Notifications
You must be signed in to change notification settings - Fork 668
feat: support newlines in tooltips #9647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support newlines in tooltips #9647
Conversation
|
@domoritz: working on this issue I noticed signal strings created by Vega-Lite all use escaped double quotes
would become
Is there a reason for not using single quotes? Maybe I'm missing some edge-cases? |
|
At least one reason would be that " are slightly easier to use with the default ' in the Vega-Lite code base. I suggested changing to " in #5361. We could do it but should merge any big pending prs first. I'm supportive of it if you would like to tackle the task (in a separate pull request of course). |
|
@domoritz I'll be offline the next two weeks, but when I'm back, I would be happy to dig into the formatting of signal strings in a future pull request (after I have finished working on the backgrounds for text marks). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a copy in #9678 because this pull request has different images than the CI would output. Please use a PAT and try whether the CI builds the examples rather than manually building them.
src/compile/mark/encode/tooltip.ts
Outdated
| expr: 'datum' | 'datum.datum' = 'datum', | ||
| ): VgValueRef { | ||
| // tooltip fields with a format property are no strings | ||
| const fieldDefWithFormat = channelDef as {field: string; type: string; format: string}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you know that the type here is {field: string; type: string; format: string}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still wrestling a bit with the elaborate type system in this repo 😅
I have replaced the cast with a type guard.
| if (fieldDefWithFormat?.type === 'nominal' && !fieldDefWithFormat.format) { | ||
| const fieldString = `datum["${fieldDefWithFormat.field}"]`; | ||
| return { | ||
| signal: `isValid(${fieldString}) ? isArray(${fieldString}) ? join(${fieldString}, '\\n') : ${fieldString} : ""+${fieldString}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to convert invalid values to strings? Can we reuse logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in line with the way formatSignalRef handles nominal fields:
return {signal: `isValid(${field}) ? ${field} : ""+${field}`};
I think to render null, undefined, or NaN as strings in tooltips and area descriptions?
I'm sorry for that. I did add an action secret and tried to trigger the CI by including |
|
All good. Our setup for external forks is a bit wonky because of some restrictions GitHub actions had (have?). I often make an internal copy to get the build to work correctly. |
|
Is it true that if there is a format, the tooltip is not a string? It's a string afterwards but I guess it's not likely to be a multi line string, right? |
The comment was unclear, I have removed it. The logic is:
I hope this answers your question? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Let's get the tests to pass and I can merge.
src/compile/mark/encode/tooltip.ts
Outdated
| ): VgValueRef { | ||
| // tooltip fields that are not nominal or have a format property are no strings | ||
| if (isFieldDef(channelDef) && 'type' in channelDef && channelDef.type === 'nominal' && !channelDef.format) { | ||
| if (isFieldDef(channelDef) && 'type' in channelDef && isDiscrete(channelDef.type) && !channelDef.format) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't 'type' in channelDef always true?
|
I merged your changes into #9678. |
|
We should make you a maintainer. Would you like to (https://github.com/vega/.github/blob/main/project-docs/MAINTAINERS.md)? |
Internal version of #9647 --------- Co-authored-by: Bas Broekhuizen <[email protected]> Co-authored-by: GitHub Actions Bot <[email protected]>
|
Thank you. Merged in #9678 |
|
@domoritz Thanks for reviewing and merging! If you have time, maybe you can have a look at the corresponding PR in vega-tooltip that is necessary to render the newlines in the tooltip. |
PR Description
This PR adds support for rendering newlines in tooltips and potentially closes #7564.
A new function has been added in the tooltip encoder that transforms tooltip values that are arrays into strings joined by newline characters. This allows multi-line tooltip content to be displayed correctly.
To support this behavior visually, a small CSS update is required in the vega-tooltip package — see the accompanying PR in that repository.
Example
With the new code, this example spec (open in editor) will render a tooltip like this: