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

Render all child components and use data-ref element #87

Merged
merged 3 commits into from
Jul 17, 2015
Merged

Conversation

cruzanmo
Copy link

This fixes the bug with overlays saving.

There were two problems:

  1. All of the child components of the component that changes need to have event handlers added
  2. We cannot assume that the first element in a component contains the data-ref attribute.
    autosave broke overlay + settings form saving #64

result.setAttribute(references.referenceAttribute, ref);
// The element with `data-ref` is not always the parent, e.g. article.
refEl = dom.find(parent, '[' + references.referenceAttribute + '=""]');
refEl.setAttribute(references.referenceAttribute, ref);
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, what's going on here?

Copy link
Author

Choose a reason for hiding this comment

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

For example, http://localhost.dev.nymag.biz:3001/components/article/instances/pr-first.html responds with:

<main role="main">
  <article class="article" data-ref="" itemscope itemtype="http://schema.org/Article">
...

We want data-ref to be added to the <article> element not the <main> element.

hmm, I could simplify the line to be refEl = dom.find(parent, '[' + references.referenceAttribute + ']');

Maybe the comment is confusing because of the use of "parent"?

Copy link
Author

Choose a reason for hiding this comment

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

@TakenPilot why does the above return with data-ref="" rather than populating that value server-side?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, that's a good question.

@cruzanmo
Copy link
Author

Discussed with @yoshokatana and @TakenPilot and agreed:

Assumption/requirement: the first element in a component should always have the data-ref property.

This allows for easy replacement of components in byline-editor. (After this PR, the article template will need its data-ref moved up to the top element)

@nelsonpecora
Copy link
Contributor

👍

container.innerHTML = target.responseText;
// The first element in a component always has the referenceAttribute.
componentEl = dom.getFirstChildElement(container);
componentEl.setAttribute(references.referenceAttribute, ref);
Copy link
Contributor

Choose a reason for hiding this comment

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

wait, why is this setting it?

Copy link
Author

Choose a reason for hiding this comment

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

Because of this #89

Copy link
Author

Choose a reason for hiding this comment

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

It is safe as long as we assume the data-ref is in the first element.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been doing this from the very beginning. Very old code. He just added a comment because it's unclear, so let's discuss tomorrow until everything is clear. I'll prepare.

I'll also add an explanation to an Issue somewhere to eventually be added to editor docs.

@nelsonpecora
Copy link
Contributor

ok 👍

cruzanmo pushed a commit that referenced this pull request Jul 17, 2015
Render all child components and use data-ref element
@cruzanmo cruzanmo merged commit 99cd52d into master Jul 17, 2015
@cruzanmo cruzanmo deleted the render-fix-2 branch July 17, 2015 16:27
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.

3 participants