Replace the header_links plugin with client-side generated anchors. #4165

bryanbraun · 2015-06-18T04:58:56Z

facebook-github-bot · 2015-06-18T05:31:44Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

Daniel15 · 2015-06-18T12:54:17Z

docs/_layouts/default.html

+                              .replace(/\s+/g, '-')
+                              .replace(/[^A-Za-z0-9\-_.]/g, '');
+
+          elements[i].setAttribute('id', tidyText);


Do the anchors actually work, since you're setting the ID after it's rendered? Does the browser correctly scroll down to the anchor when you open a fresh tab and go to a URL with an anchor?

The anchors do work. I did some testing across the various desktop browsers where I added anchors at various points in the loading process. I found that they all hop to IDs added after the DOM is fully loaded, though they differ in how much later in the process they will successfully jump to the anchor. In case you're curious, here's where each of the browsers fared on the test (the browser name is listed where they last successfully jumped to the anchor).

On DOM Ready (DOMContentLoaded)

Safari

Chrome

Opera

On Page Fully Loaded (window.onload)

Firefox

IE

After Arbitrary setTimeout

- none -

In short, we're safe in adding the IDs where we have, at least for all the browsers I tested.

facebook-github-bot · 2015-06-18T14:03:40Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

bryanbraun · 2015-06-22T03:06:02Z

OK, this is ready for review again. I also included a CSS adjustment that ensures the Sticky header doesn't obscure the heading being anchored to.

lacker · 2017-02-01T21:02:27Z

OK so this is fairly old, we redid a lot of the website since then, and there are merge conflicts. @bryanbraun does this PR still make sense and are you still interested in working on it? Or should we just deem it "timed out" and close it.

gaearon · 2017-02-02T00:30:07Z

Btw this is the oldest open PR in React repo :-)

bryanbraun · 2017-02-06T04:39:37Z

@lacker, thanks for following up. I'd like to try rebasing it later this week, and seeing if this kind of change still makes sense with the updated site. I'll let you know what I find by end of day Friday.

Fixes #4124

bryanbraun · 2017-02-10T22:34:50Z

@lacker, I've rebased the PR so it should apply cleanly now. The code is in the same place as it was in the original PR (before the closing body tag) but it can be moved if needed.

I did a bit of testing in Chrome and IE9 and haven't found any functional or visual issues. I also ran a jekyll build and confirmed that the hash-links aren't showing up in the feed.xml file anymore.

lacker

Just a few small comments. Thanks for fixing this up!

lacker · 2017-02-10T23:10:46Z

docs/css/react.scss

@@ -122,6 +122,7 @@ h1, h2, h3, h4, h5, h6 {
 .hash-link {
  color: $mediumTextColor;
  display: none;
+  padding-left: 8px;


Is this a UI change or is this just to compensate for something that's happening in that Redcarpet library? If it is a UI change, would you mind posting a before and after screen shot to show what you're changing here?

It's compensating... changing how we add space between the title and hash-link. The UI should look the same:

lacker · 2017-02-10T23:13:41Z

docs/_layouts/default.html

@@ -62,6 +62,36 @@

  <div id="fb-root"></div>
  <script>
+    (function() {


It seems inconsistent to inline this when things like live_editor.js are included as separate scripts, what do you think about refactoring this out similar to the other stuff?

I suspect people in the future will be digging through old code and trying to figure out what the heck this bit is doing, so would you mind adding just enough overall comments to explain what it is that this section of code is trying to accomplish?

Good call. I've made these updates in 1757114.

Also adds a couple comments, for context.

bryanbraun · 2017-02-14T16:42:07Z

Ok, that should be all the changes you requested. 👍

lacker · 2017-03-01T19:53:40Z

Thank you! This seems a bit less technical debty, in addition to working better with RSS, since I have a sneaking suspicion that React folks would prefer to write javascript than ruby.

gaearon · 2017-03-01T23:52:46Z

😮

…4165) * Replace the header_links plugin with client-side generated anchors. Fixes #4124 * Move anchor-link code into a separate script Also adds a couple comments, for context. (cherry picked from commit 7a878d2)

facebook-github-bot added the GH Review: review-needed label Jun 18, 2015

bryanbraun mentioned this pull request Jun 18, 2015

Omit anchors for headers in feed? #4124

Closed

Daniel15 reviewed Jun 18, 2015
View reviewed changes

facebook-github-bot added the CLA Signed label Jun 18, 2015

zpao added the Component: Documentation & Website label Jul 13, 2015

ghost added the CLA Signed label Jul 12, 2016

aweary assigned lacker Jan 26, 2017

Replace the header_links plugin with client-side generated anchors.

55062a3

Fixes #4124

lacker suggested changes Feb 10, 2017

View reviewed changes

Move anchor-link code into a separate script

1757114

Also adds a couple comments, for context.

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

lacker merged commit 7a878d2 into facebook:master Mar 1, 2017

lacker added Documentation: needs merge to stable and removed Component: Documentation & Website labels Mar 1, 2017

gaearon removed the Documentation: needs merge to stable label Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the header_links plugin with client-side generated anchors. #4165

Replace the header_links plugin with client-side generated anchors. #4165

bryanbraun commented Jun 18, 2015

facebook-github-bot commented Jun 18, 2015

Daniel15 Jun 18, 2015

bryanbraun Jun 22, 2015

facebook-github-bot commented Jun 18, 2015

bryanbraun commented Jun 22, 2015

lacker commented Feb 1, 2017

gaearon commented Feb 2, 2017

bryanbraun commented Feb 6, 2017

bryanbraun commented Feb 10, 2017 •

edited

Loading

lacker left a comment

lacker Feb 10, 2017

bryanbraun Feb 11, 2017

lacker Feb 10, 2017

bryanbraun Feb 11, 2017

bryanbraun commented Feb 14, 2017

lacker commented Mar 1, 2017

gaearon commented Mar 1, 2017

Replace the header_links plugin with client-side generated anchors. #4165

Replace the header_links plugin with client-side generated anchors. #4165

Conversation

bryanbraun commented Jun 18, 2015

facebook-github-bot commented Jun 18, 2015

Daniel15 Jun 18, 2015

Choose a reason for hiding this comment

bryanbraun Jun 22, 2015

Choose a reason for hiding this comment

facebook-github-bot commented Jun 18, 2015

bryanbraun commented Jun 22, 2015

lacker commented Feb 1, 2017

gaearon commented Feb 2, 2017

bryanbraun commented Feb 6, 2017

bryanbraun commented Feb 10, 2017 • edited Loading

lacker left a comment

Choose a reason for hiding this comment

lacker Feb 10, 2017

Choose a reason for hiding this comment

bryanbraun Feb 11, 2017

Choose a reason for hiding this comment

lacker Feb 10, 2017

Choose a reason for hiding this comment

bryanbraun Feb 11, 2017

Choose a reason for hiding this comment

bryanbraun commented Feb 14, 2017

lacker commented Mar 1, 2017

gaearon commented Mar 1, 2017

bryanbraun commented Feb 10, 2017 •

edited

Loading