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

Implement ChordDiagram #826

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Implement ChordDiagram #826

wants to merge 1 commit into from

Conversation

martijnversluis
Copy link
Collaborator

Resolves #820

@isaiahdahl
Copy link

isaiahdahl commented Oct 28, 2024

Somehow we're going to need the ability to get the width and height from what is rendered so that we can properly increment this.y and this.x to put multiple diagrams beside each other.

Think it might make more sense to have a width and height property on the JsPDFRenderer

so instead of

const renderer = new JsPDFRenderer(this.doc, { x: 50, y: 50, scale: 0.1 });

it could be

const renderer = new JsPDFRenderer(this.doc, { x: 50, y: 50, width: 40 });

This could just internally just calculate the "scale" against whatever the defined "width" was inside the ChordDiagram class.
Maybe we made it so that you either define width or height, but not both?

Related to properly incrementing this.x and this.y what if we create a measure() function?

What if the general idea was something along the lines of this:

Note how in this example I even removed the setting of x and y from the JsPDFRenderer and moved that to props in the diagram.render() so we could share some config elements but render multiple beside eachother

renderChordDiagrams(): void {
    const renderer = new JsPDFRenderer(this.doc, { width: 35 });

    // Assuming this would be getting ChordDiagrams from the song 
    const chordDiagrams = [
      new ChordDiagram({
        ...
      }),
      new ChordDiagram({
        ...
      }),
    ];

    chordDiagrams.forEach((diagram) => {
      const { width, height } = diagram.measure(); // or getDimensions()
      
      if (this.x + width > this.columnWidth) {
        this.y += height + 10 // diagram y spacing configurable
        this.carriageReturn();
      }

      this.x += width + 10 // diagram x spacing configurable;
      
      if (this.y + height > this.doc.internal.pageSize.getHeight()) {
        // Move to the next column if the diagram doesn't fit
        // problem with not creating a new "render" instance is that the this.doc won't be updated to hold new page if it moves to next page
        // render functon should either get a new instance, or allow this.doc to be passed into the render function 
        this.moveToNextColumn();
      } 

      diagram.render(renderer,{ x: this.x, y: this.y}, this.doc);
    });
  }
}

^ actual code would be different because I know this would have a couple bugs with extra spacing and not left aligning the first diagram but mostly I'm just trying to identify how I know we'll need to use it.

thoughts?

@martijnversluis
Copy link
Collaborator Author

@isaiahdahl I changed the rendering a bit. I removed all spacing around the diagram, and you can now set the desired width instead of the scale. The renderer exposes the renderedHeight.

@martijnversluis martijnversluis force-pushed the chord-diagrams branch 3 times, most recently from 2fb03d9 to 79f4486 Compare November 5, 2024 20:28
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.

💡 RFC: Chord Diagrams
2 participants