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

ability to have separate rendered props in a graph #220

Merged
merged 17 commits into from
Jul 31, 2024

Conversation

dqnykamp
Copy link
Member

This PR extends the for_render attribute on a prop so one can separately specify if the prop is for_render when the component is in a graph versus in text.

The PR also adds a separate point renderer and rendered props when the point is not in a graph.

@dqnykamp

This comment was marked as outdated.

@dqnykamp dqnykamp requested a review from siefkenj July 18, 2024 02:19
// Determine whether or not component is in a graph (i.e., has a graph ancestor).
// If so, we'll look for props that are rendered `in_graph` rather than `in_text`.
let in_graph = document_structure
.get_true_component_ancestors(component_idx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't recall what this function exactly does, but does the _true mean it ignores parents coming from extend? Please verify that this is actually the call we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. It gets the parent from the DAST and skips extra parents such as the component extended.

Copy link
Contributor

Choose a reason for hiding this comment

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

But is that what we want? If we have a point in a graph and we extend that point outside of a graph, we want the one outside the graph to use the text renderer.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. That's what this should do, given that the "true" ancestors of the extended point will not include a graph. (In the dast, the extended point doesn't descend from a graph, though it does descend from a graph via its extended link from its source.)

I didn't rely on my understanding; I tested examples. I now made an automated test demonstrating this: rendered_props_with_references() inside the point tests. Let me know if there is another case I should consider.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand now. We do want the true parent and not the inherited one! Tests give me confidence :-)

@siefkenj
Copy link
Contributor

Looks good to go :-)

@dqnykamp dqnykamp merged commit b1d95c7 into Doenet:0.9 Jul 31, 2024
4 checks passed
@dqnykamp dqnykamp deleted the render-outputs branch August 14, 2024 20:04
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.

2 participants