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

Create Chart class that all charts inherit from #32

Closed

Conversation

laimonasA
Copy link
Collaborator

@laimonasA laimonasA commented Dec 2, 2019

Relates to: #32

Created a super class called Chart, it has the common properties in all charts, (if I missed any please let me know and I'll add it!)

Also added the following methods which are identical in all charts:

  • initChartValues
  • setSvg
  • resolveFont
  • resolveData
  • setTitle

These two methods: drawFromFile and drawFromObject MUST be implemented when extending the Chart class.

@laimonasA
Copy link
Collaborator Author

To try and keep all of the configuration the same, I have added a lot of customisation to some of the methods. Arguably this has added additional complexity so I would prefer to make it more general, e.g width/height of the graph.

What do you think @jwilber ?

@laimonasA
Copy link
Collaborator Author

This is not yet ready, just noticed an issue.

@laimonasA laimonasA closed this Dec 4, 2019
@jwilber
Copy link
Owner

jwilber commented Dec 9, 2019

Sorry for response time, I've been traveling!
I think this looks awesome, and is exactly what we're looking for here.

P.S. - I've added you as a collaborator since you've been doing some good work and seem interested :)

@laimonasA
Copy link
Collaborator Author

Can I standardize the default parameters for the charts, e.g. height and width, (roughness for some)?

@jwilber
Copy link
Owner

jwilber commented Dec 9, 2019

What exactly do you mean by standardize in this instance? Like naming conventions? Or do you mean standardize the default values across the charts? I think either is fine, just tag me on the PR.

@laimonasA
Copy link
Collaborator Author

I mean the default values yeah, some are larger than others and such.

Alright, I'll do that as a part 1 prep. PR and tag you in it.

Thanks

@jwilber
Copy link
Owner

jwilber commented Dec 9, 2019

Sounds great.
I was also thinking about making the charts responsive.

As far as I can tell, this may be done by changing the width and height attributes for this.svg in the setSvg() method to preserveAspectRatio and viewBox as follows:

Old:

setSvg() {
    this.svg = select(this.el)
      .append('svg')
      .attr('width', this.width + this.margin.left + this.margin.right)
      .attr('height', this.height + this.margin.top + this.margin.bottom)
      .append('g')
      .attr('id', this.roughId)
      .attr('transform',
        'translate(' + this.margin.left + ',' + this.margin.top + ')');
  }

New

setSvg() {
    this.svg = select(this.el)
      .append('svg')
      .attr("preserveAspectRatio", "xMinYMin meet")
     .attr("viewBox", `0 0 ${(this.width + this.margin.left + this.margin.right)}
       ${(this.height + this.margin.top + this.margin.bottom)}`)
      .append('g')
      .attr('id', this.roughId)
      .attr('transform',
        'translate(' + this.margin.left + ',' + this.margin.top + ')');
  }

I can wait to make this change after you've finished the ABC, but since you're working on it (and I believe setSVG() will be a method of the ABC), I figured you could add it, assuming it works.

Something to think about!

@laimonasA laimonasA deleted the refactor/create-chart-super-class branch December 28, 2019 13:32
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.

None yet

2 participants