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

DonutChart → Donut, DonutSlice → Donut.Slice #269

Merged
merged 16 commits into from
Sep 28, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Sep 20, 2018

This is a proposal to help us decide on #166. It renames the DonutChart component to Donut, moves the DonutSlice component into src/Donut.js, and makes it available as Donut.Slice. Before:

import {DonutChart, DonutSlice} from '@primer/components'
export default () => (
  <DonutChart>
    <DonutSlice state="pending" value="3" />
    <DonutSlice state="success" value="2" />
  </DonutChart>
)

After:

import {Donut} from '@primer/components'
export default () => (
  <Donut>
    <Donut.Slice state="pending" value="3" />
    <Donut.Slice state="success" value="2" />
  </Donut>
)

And if this still seems too verbose, we could ditch the Slice component altogether and allow only the data prop.

@vercel
Copy link

vercel bot commented Sep 20, 2018

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@shawnbot shawnbot requested a review from a team September 20, 2018 16:07
Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

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

Just left a few questions! I'm cool with this method but would like to see what the others think. If everyone is on board let's do a major release next week with this & the Link work in it.

pages/components/docs/Donut.md Show resolved Hide resolved
src/Donut.js Show resolved Hide resolved
pages/components/docs/Donut.md Show resolved Hide resolved
@shawnbot shawnbot requested a review from a team September 21, 2018 06:56
@shawnbot shawnbot changed the base branch from q3-lochness-monster to master September 22, 2018 03:41
@emplums emplums changed the base branch from master to q3-wildflower September 25, 2018 22:12
@emplums
Copy link

emplums commented Sep 27, 2018

@broccolini @jonrohan do either of you have any thoughts on this? If everyone feels good about this PR we'll use it to model other parent/child components

@shawnbot shawnbot added the major release breaking changes label Sep 27, 2018
@emplums
Copy link

emplums commented Sep 28, 2018

@shawnbot I feel good about merging this for now so we can have it in this week's release - was hoping we could discuss this model for parent/child elements as a team so we're all on the same page, maybe for next week's review?

@emplums emplums mentioned this pull request Sep 28, 2018
6 tasks
@emplums emplums merged commit 027b377 into q3-wildflower Sep 28, 2018
@emplums emplums deleted the child-components branch September 28, 2018 19:07
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.

2 participants