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(xo-web): add donut chart with legends component #7778

Closed
wants to merge 6 commits into from

Conversation

P4l0m4
Copy link
Collaborator

@P4l0m4 P4l0m4 commented Jun 26, 2024

Capture d'écran 2024-07-04 111118

@P4l0m4 P4l0m4 requested a review from OlivierFL July 3, 2024 15:32
@OlivierFL OlivierFL requested a review from ByScripts July 4, 2024 08:06
Comment on lines 1 to 5
export type DonutColor = 'success' | 'warning' | 'error' | 'unknown'
export type DonutSegment = {
value: number
color: DonutColor
}
Copy link
Contributor

Choose a reason for hiding this comment

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

DonutSegmentColor would probably be a more suitable name.

Suggested change
export type DonutColor = 'success' | 'warning' | 'error' | 'unknown'
export type DonutSegment = {
value: number
color: DonutColor
}
export type DonutSegmentColor = 'success' | 'warning' | 'error' | 'unknown'
export type DonutSegment = {
value: number
color: DonutSegmentColor
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You can name the file legend.type.ts

label: string
value: number
unit?: string
color: LegendColor
Copy link
Contributor

Choose a reason for hiding this comment

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

DonutSegmentColor being more restrictive than LegendColor, it would therefore make more sense to use it.

Suggested change
color: LegendColor
color: DonutSegmentColor

Comment on lines 46 to 65
const donutSegments = computed(() => {
if (props?.segments.length === 0) {
return []
}
return props.segments
.filter(segment => segment.color !== 'dark-blue')
.map(segment => {
let color
if (segment.color === 'disabled' || segment.color === 'default') {
color = 'unknown'
} else {
color = segment.color
}

return {
value: segment.value,
color,
}
}) as DonutSegment[]
})
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. props always exists, the ? is useless.
  2. No need to check for segments.length. If props.segments is empty, then .filter.map will return an empty array anyway.
  3. It makes no sense to accept dark-blue as segment color if these segments are then completely excluded from the result.
Suggested change
const donutSegments = computed(() => {
if (props?.segments.length === 0) {
return []
}
return props.segments
.filter(segment => segment.color !== 'dark-blue')
.map(segment => {
let color
if (segment.color === 'disabled' || segment.color === 'default') {
color = 'unknown'
} else {
color = segment.color
}
return {
value: segment.value,
color,
}
}) as DonutSegment[]
})
const donutSegments = computed(() =>
props.segments.map(segment => ({
value: segment.value,
color: segment.color === 'unknown' ? 'disabled' : segment.color,
} as DonutSegment))
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think nothing prevents this component from being moved to Web Core.

@P4l0m4 P4l0m4 force-pushed the donut-with-legends branch 2 times, most recently from 4dd7d0f to 2cbb479 Compare July 10, 2024 09:30
@ByScripts
Copy link
Contributor

Superseded with #7829

@ByScripts ByScripts closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants