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

Allow clients to add more attributes to html output #96

Closed
wants to merge 5 commits into from

Conversation

clemens-tolboom
Copy link
Collaborator

This PR adds

public function createImageObject() {

in order to allow clients to add more attributes to render their graph html like

$domElement = $graphviz->createImageObject();
$domElement->setAttribute('style', 'max-width:100%;');
return $domElement->ownerDocument->saveHtml($domElement);

@@ -228,10 +228,10 @@ public function createImageSrc()
public function createImageHtml()
{
if ($this->format === 'svg' || $this->format === 'svgz') {
return '<object type="image/svg+xml" data="' . $this->createImageSrc() . '"></object>';
return '<object width="100%" type="image/svg+xml" data="' . $this->createImageSrc() . '"></object>';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One could argu the client should call createImageSrc instead.

But I prefer an attributes approach.

@clemens-tolboom
Copy link
Collaborator Author

Hmmm ... this is too tricky.

In Drupal I had two groups (1 class and the other 10 classes) of which each image renders 100% width ... that looks bad.

@clemens-tolboom
Copy link
Collaborator Author

I want to merge this today as:

  1. There are tests for the change
  2. We get test capabilities for Graphviz dot rendering
  3. There is no BC problem
  4. I need this for Drupal

@clue
Copy link
Member

clue commented Jan 3, 2014

Thanks for the PR! I can perfectly understand where you're coming from and certainly see a need for something like this.

I want to merge this today

Thanks for the heads up. However, I think we should reconsider if we actually want this in this library. We've already discussed moving GraphViz support to a separate project (which I'm not opposed against at all). Also, your changes add a requirement for ext-dom (which is not available on at least one of my installations).

Perhaps it makes sense to remove createImageHtml() altogether? What's your position on that matter?

@clemens-tolboom
Copy link
Collaborator Author

discussed moving GraphViz

Yeah ... we should come up with a good package diagram. What belongs were!

Moving Graphviz out of Graph is a good idea. Puzzle is where to? A new repo? I have no good suggestion for it's new home :-/

requirement for ext-dom

Adding ext-dom to the plate is indeed a nimby dependency for Graph. But for Graphviz we want to add HTML like stuff then ext-dom is not bad IMHO ... I'm working in a test for graph-uml clue/graph-uml#23

remove createImageHtml()

I like the code flow as setFormat decide what the output html should be. So please leave it in.

@clemens-tolboom
Copy link
Collaborator Author

Shall we add the PR apart from the functional changes in

 function createImageHtml()

Then we have the nice createImageObject for CMS clients like Drupal. Or does that break installation on non ext-dom systems?

@clue
Copy link
Member

clue commented Mar 22, 2014

As discussed in #97, GraphViz will be moved to a separate repo (graphp/graphviz) eventually.

As such, this feature doesn't make much sense in this library anymore. I'm inclined to close this ticket, but I'll leave this open for reference until #97 is completed.

@clemens-tolboom
Copy link
Collaborator Author

Let's not loose this PR for graphviz :)

@clue
Copy link
Member

clue commented Dec 31, 2014

As noted above, GraphViz will be split off into a separate repository, see graphp/graphviz#1.

Let's not loose this PR for graphviz :)

I've moved the suggestion over to graphp/graphviz#4, so we can now close this here. Thanks for the suggestion!

@clue clue closed this Dec 31, 2014
@clue clue deleted the graphviz-result-sizes branch January 1, 2015 12:41
@clemens-tolboom
Copy link
Collaborator Author

Hope to find time to revive my PR but afraid not :(

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

Successfully merging this pull request may close these issues.

2 participants