LG-4659: Replace axes unit prop with formatter
#2536
Merged
+64
−55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 5447e1b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tsck
commented
Nov 1, 2024
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@lg-charts/core': minor | |||
There was a problem hiding this comment.
I guess this is technically major since it's not backwards compatible, but since it's still in v0, I made it minor. Let me know if this is incorrect, though!
There was a problem hiding this comment.
Yeah, pre-v1 changesets can be a bit loose. Rule of thumb for me is everything shifts down a level
tsck
commented
Nov 1, 2024
charts/core/README.md
Outdated
| | ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | ------- | | ||
| | `type` | Type of axis. | 'category' \| 'value' \| 'time' \| 'log' | | | ||
| | `label` _(optional)_ | Label name to be rendered on the axis. | string | | | ||
| | `formatter` _(optional)_ | Formatter of axis label, which supports string template and callback function. (See [ECharts xAxis.axisLabel.formatter docs](https://echarts.apache.org/en/option.html#xAxis.axisLabel.formatter) for more details) | string \| ((value: string, index: number) => string) \| { [key: string]: string } | | |
There was a problem hiding this comment.
Is it weird to link out to the ECharts docs in our docs? 🤔
There was a problem hiding this comment.
Not strange to link out, but we might want to be careful with the wording. Do we want to commit ourselves to sticking with the ECharts API, or do we want to give ourselves more freedom? (I think this is fine, but just want to be careful about implying that our API is exactly the same as theirs)
There was a problem hiding this comment.
Addressing this more in thread here (TL;DR - I think this probably should be removed, and we should only publicly support a subset for this)
|
Size Change: -71 B (-0.01%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
| @@ -87,17 +84,17 @@ const getOptions = ({ | |||
| * <XAxis | |||
| * type="time", | |||
| * label="My X-Axis Data", | |||
| * unit="GB" | |||
| * formatter="{value}GB" | |||
There was a problem hiding this comment.
is this correct? Oh I see now. I was expecting a template literal. This is how ECharts interpolates strings?
There was a problem hiding this comment.
Yup, exactly. Specifically for rendering the value. There are other formatters such as '{yyyy}-{MM}-{dd}' as well.
charts/core/src/XAxis/XAxis.types.ts
Outdated
| formatter?: | ||
| | string | ||
| | ((value: string, index: number) => string) | ||
| | { [key: string]: string }; |
There was a problem hiding this comment.
What would an object formatter look like?
There was a problem hiding this comment.
The object formatter is what echarts calls a "cascading template" and only works with time. From their site:
Sometimes, we wish to use different formats for different time granularity. For example, in a quarter-year chart, we may wish to see the month name with the first date of the month, while see the date name with others. This can be made with:
formatter: { month: '{MMMM}', // Jan, Feb, ... day: '{d}' // 1, 2, ... }
I guess that means this could actually be typed better:
Suggested change
| | { [key: string]: string }; | |
| | { [key: CascadingTemplateLevels]: string }; |
where CascadingTemplateLevels is defined as:
type CascadingTemplateLevels =
| 'year'
| 'month'
| 'day'
| 'hour'
| 'minute'
| 'second'
| 'millisecond'
| 'none';However, I think given your point in this comment, I'm wondering if we should publicly support this. Given that EChart's string interpolation isn't standard JS either, I'm almost wondering if we should just publicly support the callback function. Consumers can still get hacky, override the type and use either of these, but by not publicly supporting them it probably helps save some headaches in the future if we ever need to switch the underlying library. What do you think?
There was a problem hiding this comment.
Decided to make the decision to only support the callback function and have pushed changes to reflect that. Let me know if there's any disagreement here.
bruugey
reviewed
Nov 1, 2024
charts/core/README.md
Outdated
| @@ -26,7 +26,7 @@ import { Chart, Line, Grid, XAxis, YAxis } from '@lg-charts/core'; | |||
| <Chart> | |||
| <Grid vertical={false}> | |||
| <XAxis type="time" /> | |||
| <YAxis type="value" unit="GB" /> | |||
| <YAxis type="value" formatter="{value}GB" /> | |||
There was a problem hiding this comment.
this api does feel a little tied to echarts - should we leverage a more expected API and then normalize it on our side before sending thru
There was a problem hiding this comment.
Certainly open to discussion here, just wanted to call it out
There was a problem hiding this comment.
Agreed! I'm thinking that we just publicly expose the callback. Addressing this more in this thread
| * /> | ||
| * </Chart> | ||
| */ | ||
| export function YAxis({ type, label, unit }: YAxisProps) { | ||
| export function YAxis({ type, label, formatter }: YAxisProps) { |
There was a problem hiding this comment.
Looks like there's a lot of similarity between XAxis & YAxis. Not necessary in this PR, but could be worth DRY-ing this up a bit
There was a problem hiding this comment.
Yup, they're almost identical atm with just a few different props. I considered having a more common component that these two work off of but decided not to since there are only 2, but I agree, it probably makes sense. I'll move in this direction in a future PR.
TheSonOfThomp
approved these changes
Nov 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
unitprop ofXAxisandYAxiswithformatterprop.🎟 Jira ticket: LG-4659
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes