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

Only process footnote links if component is mounted #63

Merged
merged 3 commits into from
Feb 21, 2018
Merged

Conversation

eatyourgreens
Copy link
Contributor

  • Checks for a root DOM node before querying the HTML content to find footnote links.
  • Removes displayName.
  • Replaces findDOMNode with a ref.

@eatyourgreens
Copy link
Contributor Author

@simensta any objections if I merge this?

@simensta
Copy link
Contributor

The code builds and the tests run successfully for me, but I am having trouble using npm link to test the package in Panoptes-Front-End. Is there another way to test the package in PFE?

@eatyourgreens
Copy link
Contributor Author

Good question. I don't actually know how to test a build of markdownz, come to think of it.

@simensta
Copy link
Contributor

I am now able to use npm link to test the package in PFE. However, I get the following error:

Uncaught Error: addComponentAsRefTo(...): Only a ReactOwner can have refs. 
You might be adding a ref to a component that was not created inside a component's `render` method,
or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner).

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Feb 21, 2018

npm link is working for me with a local copy of PFE so I'm not sure why that error is coming up. In the past, that error has been thrown when PFE and markdownz are using different versions of React. Both should be on 15.6.2. Does npm ls react show different versions installed for PFE?

npm install path might be a better alternative, if npm link isn't working.

npm install folder:

Install the package in the directory as a symlink in the current project. Its dependencies will be installed before it's linked. If sits inside the root of your project, its dependencies may be hoisted to the toplevel node_modules as they would for other types of dependencies.
https://docs.npmjs.com/cli/install

@simensta simensta self-requested a review February 21, 2018 17:25
Copy link
Contributor

@simensta simensta left a comment

Choose a reason for hiding this comment

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

My npm link setup is finally working! 🎉 Thank you for your help!

I worked through the classification interface (submitting a classification and loading a new subject) looking for the errors mentioned here: zooniverse/Panoptes-Front-End#4324 (comment). I didn't see any DOM warnings or console warnings pertaining to markdown footnotes.

During this review, I noticed that the footnote feature doesn't display the footnote text. This is true of this branch as well as markdownz master. I'll create an issue so that can be addressed separately.

@simensta simensta merged commit 82d3b18 into master Feb 21, 2018
@simensta simensta deleted the footnote-links branch February 21, 2018 17:43
@eatyourgreens
Copy link
Contributor Author

Published as 7.4.2.

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