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

Adding tests and revisions to PR #560 : Provide programmatic column header for table, stock example updated #668

Merged
merged 12 commits into from
Sep 29, 2014

Conversation

axiomabsolute
Copy link

Modified Object-based column definition to use objects of the form {label: "..", format: function(d){/**/}}

Added Jasmine tests to cover:

  1. Labels only
  2. Functions only
  3. Objects

Chris Meier and others added 12 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().
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.
@gordonwoodhull
Copy link
Contributor

Hey @axiomabsolute, didn't know this was you! Thanks for contributing!

If I don't manage to merge this soon, it is just because I am offline.. Just branch off this branch and continue working.

Trust me, it'll work better than separate feature branches, since I know you have other plans for the same few files.

@gordonwoodhull
Copy link
Contributor

(I'm mostly offline in the woods but can't quite stay away from dc!)

@mr23
Copy link
Contributor

mr23 commented Aug 9, 2014

Devon, thanks for finishing this.

On August 8, 2014 9:49:50 AM CST, Devon Terrell notifications@github.com wrote:

Modified Object-based column definition to use objects of the form
{label: "..", format: function(d){/**/}}

Added Jasmine tests to cover:

  1. Labels only
  2. Functions only
  3. Objects
    You can merge this Pull Request by running:

git pull https://github.com/axiomabsolute/dc.js master

Or you can view, comment on it, or merge it online at:

#668

-- Commit Summary --

  • table header moved to dc.js
    • changed data-table to support dynamically set columns for table
      header
  • fixing unintended checkins
  • updated stock example via grunt
  • attempt again, renderedstockfixture
  • attempt3 after installing saucelab; touched data-table.js
  • retry 4. connected sauce
  • review changes
  • updated restored stock example
  • added ValueFormat function
  • Removed columnHelper fn

-- File Changes --

M regression/rendered-stock-fixture.html (4)
M spec/data-table-spec.js (47)
M src/data-table.js (140)
M web/index.html (2)
M web/stock.js (107)

-- Patch Links --

https://github.com/dc-js/dc.js/pull/668.patch
https://github.com/dc-js/dc.js/pull/668.diff


Reply to this email directly or view it on GitHub:
#668

Sent from my Android device with K-9 Mail. Please excuse my brevity.

@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Sep 29, 2014
@gordonwoodhull gordonwoodhull merged commit a208a90 into dc-js:master Sep 29, 2014
gordonwoodhull added a commit that referenced this pull request Sep 29, 2014
gordonwoodhull added a commit that referenced this pull request Sep 29, 2014
@gordonwoodhull
Copy link
Contributor

Thanks @mr23 and @axionabsolute! This is a useful feature.

Our linters were complaining about the unused functions in the stock example, so I decided it was best just to keep the most general example, which also meant I could remove the header from index.html.

Apparently firefox doesn't support innerHTML (who knew it's an IE thing?), so I swapped for textContent and the tests run clean on all five browsers.

@gordonwoodhull
Copy link
Contributor

Oh. I'm going to have to fix those _doCustom methods. That's not how we do customization points around here! Still, I think the functionality is solid, so it shouldn't be a big change.

The code isn't entirely consistent, but the majority of the code has a standard of having _functions be exclusively internal customization points – where base classes/mixins communicate with derived classes. Public methods should not be underscored. Anything that is customizable should have a public getter/setter.

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