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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CircleGraph #458

Open
wants to merge 1 commit into
base: newstyle
from

Conversation

@deamme
Copy link
Contributor

commented Aug 12, 2019

Intro

Some changes that I needed for the presale functionality of the Fundraising app.
You might or might not want these changes, but here they are 馃槂

Changes

  • Font size is now dynamic based on the size
  • Changed font color to #25314d
  • Border look has been changed to a grey outline
  • Added color prop
  • Added width prop to change the stroke width

Preview

CircleGraph

@auto-assign auto-assign bot requested a review from bpierre Aug 12, 2019

@sohkai
Copy link
Member

left a comment

Thanks @deamme! A couple suggestions to improve this component and make it generic enough for you (and others!) 馃槃

* Type: `Number`
* Default: `4`

Width of the circle.

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member
Suggested change
Width of the circle.
Stroke width of the circle.

I'd also consider renaming this prop to stroke or etc. "Width" of a circle usually means diameter which is definitely not what we want to convey 馃槃.

@@ -7,11 +7,12 @@ const BORDER_WIDTH = 4

const VALUE_DEFAULT = 1
const SIZE_DEFAULT = 80
const LABEL_DEFAULT = value => `${Math.round(value * 100)}%`
const LABEL_DEFAULT = value => `${Math.round(value * 100)}`

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

I'd leave the % here; a user might want to display a non-percentage label (e.g. step 1 out of 3).

const radius = (size - BORDER_WIDTH) / 2
const CircleGraph = ({ value, label, size, color, width }) => {
const length = Math.PI * 2 * (size - width)
const radius = (size - width) / 2

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

Small nitpick; perhaps we can re-order this so it's more math-like:

const radius = (size - width) / 2
const circumference = Math.PI * 2 * (2 * r)
`}
>
%
</Label>

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

Rather than using two labels, perhaps we can use a renderLabel prop or etc. to let the user plug whatever label they'd like.

We can keep the simple previous one as a default.

@@ -72,21 +94,20 @@ const CircleSvg = styled.svg`

const CircleBase = styled.circle`
fill: none;
stroke: #6d777b;
opacity: 0.3;
stroke: #ecedf1;

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

We probably want theme.accent for this instead?

font-weight: 400;
color: #000;
color: #25314d;

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 13, 2019

Member

We should make this a theme colour; perhaps theme.surfaceContent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.