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.

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

Passing DOM element to Factory's renderer.elementId param #580

Open
devboell opened this issue Oct 7, 2017 · 3 comments · May be fixed by #1615
Open

Passing DOM element to Factory's renderer.elementId param #580

devboell opened this issue Oct 7, 2017 · 3 comments · May be fixed by #1615
Labels
feature request

Comments

@devboell
Copy link

devboell commented Oct 7, 2017

I am wondering about the EasyScore example, and how you configure the renderer by means of the Factory.

Because I am using React I don't want to use getElementById, but get the DOM element with React Component's ref attribute

Although the Factory explicitly asks for a elementId, I see in Renderer that it is still ok to pass the element itself.

    this.element = document.getElementById(elementId);
    if (!this.element) this.element = elementId;

Can I safely assume this option will remain available in the future?
@devboell devboell changed the title Factory, DOM element and React Passing DOM element to Factory's renderer.elementId param Oct 7, 2017
@allan-simon
Copy link

wouldn't it be best to add a new option "element" to make it more explicit (while keeping the possibility to give element as elementId ?, so that you don't break backward compatibility )

@alindsay55661
Copy link

I also would like to use (rely on) this option so would appreciate a formal acknowledgment in the API. Even the ability to pass a Renderer directly (instead of renderer configuration) would be great. Either of these would be great:

// Factory 'renderer' config accepts an 'element' property as a DOM element
const vf = new Vex.Flow.Factory({renderer: { element: vexElementRef.current }});
// Factory 'renderer' option accepts a vex renderer instance
const renderer = new Renderer(vexElementRef.current, Renderer.Backends.SVG);
const vf = new Vex.Flow.Factory({renderer});

@rvilarl
Copy link
Collaborator

rvilarl commented Sep 25, 2022 •
edited

Looking into that we do have a inconsistency between the parameter used by:

  • Factory which is defined as string | null , and
  • Renderer which is defined as string | HTMLCanvasElement | HTMLDivElement | RenderContext

It would make sense to accept the same possibilities in Factory, wouldn't it?

@rvilarl rvilarl mentioned this issue Sep 29, 2022
Closed
MGreek added a commit to MGreek/vexflow that referenced this issue Apr 2, 2024
@MGreek
feat(Factory): allow initilalization with Renderer
140e270 
The `renderer` field of `FactoryOptions` was changed to also be an
instance of `Renderer` via an union.

The other way of initializing a `Factory` remains unaffected.

A test-case had to be slightly modified since it didn't account for
the union I created.

This fixes 0xfe#580 since an instance of `Renderer` can be created with DOM
elements and `Factory` can now be created with an instance of
`Renderer`.
@MGreek MGreek linked a pull request Apr 2, 2024 that will close this issue
Open
MGreek added a commit to MGreek/vexflow that referenced this issue Apr 27, 2024
@MGreek
feat(Factory): allow initialization with Renderer
5d6861b 
The `renderer` field of `FactoryOptions` was changed to also be an
instance of `Renderer` via an union.

The other way of initializing a `Factory` remains unaffected.

A test-case had to be slightly modified since it didn't account for
the union I created.

This fixes 0xfe#580 since an instance of `Renderer` can be created with DOM
elements and `Factory` can now be created with an instance of
`Renderer`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Factory initialization directly with Renderer MGreek/vexflow
4 participants
@allan-simon @alindsay55661 @devboell @rvilarl