8000 Add title config option to c3 charts core by davidpoore · Pull Request #1025 · c3js/c3 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add title config option to c3 charts core #1025

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

Jump to bottom
Merged
merged 1 commit into from
May 5, 2015
Merged

Add title config option to c3 charts core #1025

merged 1 commit into from
May 5, 2015

Conversation

davidpoore
Copy link
Contributor

For Issue #1021

Based on my work doing some local modifications to c3 for Pivotal Tracker, this code allows users to specify a title config options, passing it text, x, and/or y values and rendering it as a text node on the chart.

@davidpoore
Copy link
Contributor Author

This finished successfully on Travis, but isn't reporting duration and seems stuck in pending mode - maybe need to kick off another build?

@davidpoore
Copy link
Contributor Author

Interesting. This only fails for me locally some of the time - I did a full grunt build and jasmine run before pushing - it seems one of the draw values is calculated incorrectly on certain runs, but not others.

@davidpoore
Copy link
Contributor Author

I don't believe my changes are related to the failing arc spec. At first I was concerned that the top padding changes were messing with the math, but no title is specified in the arc tests. Using some console logs, I saw that the top padding was always 0 - with or without my changes.

To investigate further, I did a fresh clone of the masayuki0812/c3, and this intermittent failure still happens. There is something broken in the arc code that causes it to fail occasionally.

@davidpoore davidpoore closed this Feb 26, 2015
@davidpoore davidpoore reopened this Feb 26, 2015
@aendra-rininsland
Copy link
Member

Intermittent test failure notwithstanding, it's awesome you've done this. I'll try to get around to reviewing this in the next day or so.

@davidpoore
Copy link
Contributor Author

@Aendrew thanks - my other pull request (#1303) is somewhat related to this one as well. In conjunction, they allow you to have title & header and a top-right anchored legend to get a chart layout like this: https://dribbble.com/shots/1879542-Story-type-breakdown-chart?list=users&offset=2

we're using c3 at Pivotal Tracker for our new charts framework, so we wanted to contribute to the library where we can :)

@aendra-rininsland
Copy link
Member

Hi!

Quick review:

  1. Incredibly nice work overall.
  2. You included a spec! You get a cookie! 🍪
  3. Please remove the minified version, we only build those when doing a release and it's preventing you from being able to merge successfully.
  4. Please also squash your commits.
  5. The default positioning is at 0,0, which means it isn't visible unless x and y are set. This will be confusing for new users. You might be able to add a bit of CSS that sets initial position a bit better (apologies if it's there and jsFiddle is just being a pain!).
  6. It'd be nice to have a few predefined positions for text: "bottom-center", "bottom-left", "top-right", etc. That can be added later, though — this shouldn't block merging it (but it might be a route towards fixing the fifth point.).

Again, many thanks, this is really nice! 👍

@davidpoore
Copy link
Contributor Author
  1. Thanks!
  2. We're big on testing here at Pivotal :)
  3. deleted minified versions
  4. squashed commits
  5. I added some code to set the default y position to the height of the bounding text box if a y value is not provided
  6. Yeah, I agree this would be nice to have in the future.

Thanks for the review, hope this makes it in soon.

@davidpoore davidpoore force-pushed the master branch 2 times, most recently from c11f069 to 749011d Compare March 2, 2015 20:53
@aendra-rininsland
Copy link
Member

Hi again!

Default positioning is now terrific; thanks for doing that!

I should've been a little clearer; when I said "remove the minified files", I didn't so much mean git rm c3.min.* (which will remove it from the parent repo when merged), but rather something more like git checkout HEAD~1 -- c3.min.*, which will reset it to one commit behind the tip. Would you mind doing the latter, committing, squashing, and pushing again? Thanks! Apologies for the hassle!

@aendra-rininsland aendra-rininsland added the C-feature-request Category: A feature request or an enhancement label Mar 3, 2015
@davidpoore
Copy link
Contributor Author

@Aendrew ah, I see - sorry! just committed the new version

@aendra-rininsland
Copy link
Member

👍 LGTM! :shipit:

@davidpoore
Copy link
Contributor Author

@Aendrew cleaned up merge conflicts, should be ready to go any time

masayuki0812 added a commit that referenced this pull request May 5, 2015
@masayuki0812
Merge pull request #1025 from davidpoore/master
6046ee0 
Add title config option to c3 charts core
@masayuki0812 masayuki0812 merged commit 6046ee0 into c3js:master May 5, 2015
@masayuki0812
Copy link
Member

Looks nice. Thank you very much for working on this 👍 @davidpoore @Aendrew

@masayuki0812 masayuki0812 mentioned this pull request May 5, 2015
Closed
@davidpoore
Copy link
Contributor Author

@masayuki0812 awesome!

masayuki0812 added a commit that referenced this pull request May 6, 2015
@masayuki0812
Add title.position/title.padding and remove title.x/title.y - #1025 #…
4eb2dd1 
…1021
@masayuki0812
Copy link
Member

As I wrote in #1021 , I added title.position and title.padding to enable us to set its position more easily. Instead, title.x and title.y has been removed because setting title position by x and y is not intuitive and title.padding seems easier to use. Now top-left, top-center and top-right are available for title.position.

@davidpoore
Copy link
Contributor Author

@masayuki0812 yeah, that seems like a reasonable change - glad to get this functionality into the core library. I hope my other pull requests can also be merged soon :)

@Dhruva2015
Copy link

When will it be released? I need to display the title for the charts. It is urgent.
Can I take the latest c3.js and c3.css file to show title? If so, how to configure the title for chart. Please help.

@aendra-rininsland
Copy link
Member

@Dhruva2015 It's in the master branch, so just clone the repo and build it.

@nalnaji
Copy link
nalnaji commented Aug 20, 2015

Hi there, is this still functional? I tried it on my graphs today and nothing changed:

var chart = c3.generate({
  padding: 10,
  data: {
    columns: [['d1',1,2,3], ['d2',1,2,3]],
    type: 'line'
  },
  title: { 
    text: 'hello'
  }
});

@d33d33 d33d33 mentioned this pull request Aug 23, 2016
Merged
@chendachao chendachao mentioned this pull request Jul 17, 2017
Closed
@Braintelligence

Copy link

Is there an example for the usage, especially concerning the padding and position out yet?

@ppKrauss
Copy link

Hi, and about position? (center=default/left/right) top=default/button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C-feature-request Category: A feature request or an enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@davidpoore @aendra-rininsland @masayuki0812 @Dhruva2015 @nalnaji @Braintelligence @ppKrauss
0