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

Provide programmatic column header for table, stock example updated #560

Closed
wants to merge 11 commits into from
Closed

Conversation

mr23
Copy link
Contributor

@mr23 mr23 commented Apr 1, 2014

Update: ready to go, April 5.

Chris Meier added 3 commits March 30, 2014 22:45
Still allows original example to function as-is.
Allows non-hard-coded table column headers.
Supplied 3 examples of setting the columns; the original method, which
requires the fixed html code for thead/cols; another which uses mostly
the data element names which become capitalized as col headings, and
another where most of the columns have the column label specified along
with an accessor function. There is some code to strip a
numberFormat/etc from a function converted to string, an attempt to
auto-generate a column header from a function, in data-table.js,
columnString().
@mr23
Copy link
Contributor Author

mr23 commented Apr 1, 2014

I've since learned to pull a topic branch, and prior to the pull reversed the commit of the auto-built dc.js's.

@mr23
Copy link
Contributor Author

mr23 commented Apr 1, 2014

I'm not sure what is wrong, it passes the grunt test here and shows as synced.

@mr23
Copy link
Contributor Author

mr23 commented Apr 2, 2014

Installed saucelab. Retry

@mr23
Copy link
Contributor Author

mr23 commented Apr 2, 2014

Ah, went down a dark alley.
"Please note that encrypted environment variables are not available for pull requests from forks."

Perhaps someone will smile gently and provide some wisdom for a newbie.

@mr23
Copy link
Contributor Author

mr23 commented Apr 2, 2014

Ah. I see this is a common Sauce problem at the moment.
I humbly await.

@gordonwoodhull
Copy link
Contributor

Hi @mr23, sorry for this bad first PR experience! and sorry I didn't intervene sooner.

There is really nothing to worry about, I think the Sauce system changed last week and everything has been failing since. Your patch should be good if it passed the tests before that part.

I'm tied up with other deadlines this week but will review your contribution soon.

@sclevine
Copy link
Member

sclevine commented Apr 2, 2014

@mr23, sorry for the trouble!
Should be fixed in 16e6863. Would you mind rebasing against master?

@mr23
Copy link
Contributor Author

mr23 commented Apr 2, 2014

Ready

@ChrisMeierRT
Copy link

How do we go about updating the API (https://github.com/dc-js/dc.js/blob/master/web/docs/api-latest.md)? The section on columns (.columns([columnFunctionArray])) still applies as-is, but I added additional functionality.

@gordonwoodhull
Copy link
Contributor

Just add to the comment around
https://github.com/mr23/dc.js/blob/master/src/data-table.js#L169

This is a valuable contribution and I have seen other workarounds by other users.

I have two comments so far:

  • please refactor to make your column namer a function parameter. So instead of just generating column names with capitalizeString and columnString, put those into a function much the same way accessors work. What you have would be a good default but I'm sure others will want this work a different way.
  • please don't use tabs - follow the formatting of the code around it. Often this means replacing tabs with 8 spaces but look and see what it means for your code.

Chris Meier added 4 commits April 5, 2014 13:33
Restored index.html, no need for a change.
Added 4th example of columns.
so both header and value can be overridden
Changed to use just an in-line Object, and allow any Object
specification desired, with note to override the internal functions to
match.
I think this is the cleanest and most flexible version.
@mr23
Copy link
Contributor Author

mr23 commented Apr 5, 2014

Gordon, SC,
I believe this is ready to go, See comments immediately above for the groups of changes.
Let me know if you need anything further.
-Chris

// A 2nd option is a string representing a field in the data.
// A third option is to supply an Object such as an array of 'information', and
// supply your own _doColumnHeaderFormat and _doColumnValueFormat functions to
// create what you need.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment confuses me. It doesn't look like one would provide _doColumnHeaderFormat and _doColumnValueFormat, but instead would supply the header string and a format function. Is this comment obsolete?

@gordonwoodhull
Copy link
Contributor

This looks really helpful. Using the example for testing is great, but could you add jasmine tests for the following cases?

  • labels only
  • functions only
  • objects only
  • no header (is this still supported?)

For the Object formatting, I would prefer an object rather than an array. So, instead of

                ["Open",
              function (d) {
              return numberFormat(d.open);
              }],

I would prefer something like:

                {label: "Open",
              format: function (d) {
              return numberFormat(d.open);
              }},

It's a little more verbose but clearer.

Thanks!

@mr23
Copy link
Contributor Author

mr23 commented Apr 29, 2014

Okay, I'll look into it next opportunity.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Jun 5, 2014
@mr23
Copy link
Contributor Author

mr23 commented Jul 27, 2014

Going to try and pick this back up. Could it still make a cutoff for 2.0 ?

@gordonwoodhull
Copy link
Contributor

Yeah, I hope to release mid-week.

@gordonwoodhull
Copy link
Contributor

Also, there shouldn't be quite such a long wait between releases after that!

axiomabsolute pushed a commit to axiomabsolute/dc.js that referenced this pull request Aug 8, 2014
@gordonwoodhull
Copy link
Contributor

This was subsumed/replaced by #668, which adds tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants