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.

Sign up for GitHub

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: Pie: support multiple activeIds #2670

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: Pie: support multiple activeIds #2670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c63aeb6
docs: Pie: support multiple `activeId`s
assaf-aviram Nov 14, 2024
b253367
feat: Pie: support multiple `activeId`s
assaf-aviram Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions packages/pie/src/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import {
CommonPieProps,
} from './types'

const idIsActive = (id: DatumId, activeId: null | DatumId | DatumId[]) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than supporting a scalar or an array, I think it would be better to rename the property to activeIds and to support arrays exclusively.

It would also be nice to add tests about this new feature and to adjust the documentation.

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to avoid making this a breaking change to anyone using activeId. Do you think it's worth it?

Copy link
Owner

Choose a reason for hiding this comment

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

I understand, but it makes the code and typing more complex, I'd prefer to have something a bit simpler to maintain, also the name of the property is now confusing IMHO.

Array.isArray(activeId) ? activeId.indexOf(id) > -1 : activeId === id

/**
* Format data so that we get a consistent data structure.
* It will also add the `formattedValue` and `color` property.
Expand Down Expand Up @@ -138,14 +141,12 @@ export const usePieArcs = <RawDatum>({
index: arc.index,
startAngle: arc.startAngle,
endAngle: arc.endAngle,
innerRadius:
activeId === arc.data.id
? innerRadius - activeInnerRadiusOffset
: innerRadius,
outerRadius:
activeId === arc.data.id
? outerRadius + activeOuterRadiusOffset
: outerRadius,
innerRadius: idIsActive(arc.data.id, activeId)
? innerRadius - activeInnerRadiusOffset
: innerRadius,
outerRadius: idIsActive(arc.data.id, activeId)
? outerRadius + activeOuterRadiusOffset
: outerRadius,
thickness: outerRadius - innerRadius,
padAngle: arc.padAngle,
angle,
Expand Down
70 changes: 70 additions & 0 deletions storybook/stories/pie/Pie.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,76 @@ export const ControlledActiveId: Story = {
render: () => <ControlledPies />,
}

const ControlledPiesMultipleActiveIds = () => {
const [activeId, setActiveId] = useState<string>(commonProperties.data[1].id)
const [additionalActiveIds, setAdditionalActiveIds] = useState<string>([])
Copy link
Owner

Choose a reason for hiding this comment

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

The type should be string[] I think.

function addActiveId(id) {
setAdditionalActiveIds(additionalActiveIds.concat(id))
}
function removeActiveId(id) {
setAdditionalActiveIds(additionalActiveIds.filter(x => x !== id))
}
function handleCheckboxChange(id, checked) {
if (checked) addActiveId(id)
if (!checked) removeActiveId(id)
}
function handlePieClick(id) {
if (additionalActiveIds.indexOf(id) > -1) {
removeActiveId(id)
} else {
addActiveId(id)
}
}
const allActiveIds = [activeId, ...additionalActiveIds]
return (
<div>
<div
style={{
marginBottom: '10px',
}}
>
<h3>Active IDs:</h3>
{commonProperties.data.map(x => (
<div key={x.id}>
<label>
<input
type="checkbox"
checked={additionalActiveIds.indexOf(x.id) > -1}
onChange={evt => handleCheckboxChange(x.id, evt.target.checked)}
/>
{x.id}
</label>
</div>
))}
</div>
<div
style={{
width: '800px',
display: 'grid',
gridTemplateColumns: 'repeat(2, 1fr)',
}}
>
<Pie
{...controlledPieProps}
activeId={allActiveIds}
onActiveIdChange={setActiveId}
onClick={slice => handlePieClick(slice.id)}
/>
<Pie
{...controlledPieProps}
activeId={allActiveIds}
onActiveIdChange={setActiveId}
onClick={slice => handlePieClick(slice.id)}
/>
</div>
</div>
)
}

export const ControlledMultipleActiveIds: Story = {
render: () => <ControlledPiesMultipleActiveIds />,
}

const PieWithCustomLegend = () => {
const [customLegends, setCustomLegends] = useState<LegendDatum<SampleDatum>[]>([])

Expand Down
Loading