Skip to content
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: add config.numberFormatType for using custom formatter with config.numberFormat #8228

Merged
merged 11 commits into from Jun 21, 2022

Conversation

kanitw
Copy link
Member

@kanitw kanitw commented Jun 16, 2022

  • add config.numberFormatType for using custom formatter with config.numberFormat (just like there is formatType for format)

  • We can add timeFormatType later, but I think it's hairier so I wanna handle numberFormatType first.

  • You can see that the example added originally treats 1.0 as D3's format. (But with the code changes, it now treats 1.0 as a parameter of pow.)

📦 Published PR as canary version: 5.2.1--canary.8228.a549581.0

✨ Test out this PR locally via:

npm install vega-lite@5.2.1--canary.8228.a549581.0
# or 
yarn add vega-lite@5.2.1--canary.8228.a549581.0

@kanitw kanitw marked this pull request as ready for review June 16, 2022 00:49
@kanitw kanitw force-pushed the kw/configFormatType branch 3 times, most recently from da778ec to cbcb045 Compare June 16, 2022 01:22
@kanitw kanitw requested a review from domoritz June 16, 2022 01:27
@@ -121,7 +138,11 @@ export function formatCustomType({
}) {
field ??= fieldToFormat(fieldOrDatumDef, expr, normalizeStack);

if (isFieldDef(fieldOrDatumDef) && isBinning(fieldOrDatumDef.bin)) {
if (
field !== 'datum.value' && // For axis/legend, we can't correct know the end of the bin from `datum`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that this part of the code always trigger crashes if I don't add field !== 'datum.value'

-- see this example

@kanitw kanitw force-pushed the kw/configFormatType branch 2 times, most recently from 8762532 to 9fa3563 Compare June 16, 2022 20:05
src/config.ts Outdated
@@ -145,10 +145,22 @@ export interface VLOnlyConfig<ES extends ExprRef | SignalRef> {
fieldTitle?: 'verbal' | 'functional' | 'plain';

/**
* D3 Number format for guide labels and text marks. For example `"s"` for SI units. Use [D3's number format pattern](https://github.com/d3/d3-format#locale_format).
* If numberFormatType is not specified or is `"format"`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* If numberFormatType is not specified or is `"format"`,
* If `numberFormatType` is not specified or is `"format"`,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very confused by the special string "format" here. Where is this coming from?

@kanitw kanitw merged commit da27612 into next Jun 21, 2022
@kanitw kanitw deleted the kw/configFormatType branch June 21, 2022 20:23
@github-actions
Copy link

🚀 PR was released in v5.3.0-next.4 🚀

@github-actions github-actions bot mentioned this pull request Jun 21, 2022
BradyJ27 pushed a commit to BradyJ27/vega-lite that referenced this pull request Oct 19, 2023
…config.numberFormat` (vega#8228)

Co-authored-by: GitHub Actions Bot <vega-actions-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants