Remove the Sidecar and Insert Links/Buttons Elsewhere #816

skipjack · 2017-02-04T06:34:07Z

This PR is intended to address #710. Quick summary:

Removed sidecar component entirely.
Added GitHub and StackOverflow icon links to the Navigation bar, to the right of the search icon.
Created a new Gitter button displayed in the bottom right corner of the Page.

@johnstew already added a link to the blog in the Navigation bar so I didn't do anything for that

Links in navigation bar:

Gitter button:

Adding a new gitter button for opening the overlay Preventing it from overlapping the footer on scroll

bebraw · 2017-02-04T06:57:50Z

Looks good.

skipjack · 2017-02-04T18:44:02Z

@bebraw any idea what's going on with those build errors? None of them seem to be related to the changes I made with this PR...

If not, I'll try running the hyperlink test locally and reverting some of my changes to see what's causing it.

bebraw · 2017-02-04T19:00:59Z

@skipjack There are some relative links at package readmes. We would have to rewrite those to point at GitHub I think.

simon04 · 2017-02-04T19:41:04Z

👍
What might be irritating is that the GItHub+StackOVerflow links are the only two links inside the navigation bar that leave https://webpack.js/org/

skipjack · 2017-02-05T00:58:31Z

@simon04 yea I see what you mean but I think it's fine, both React and Babel have a similar implementation.

@bebraw so should we merge and solve this in another PR? Or if you point me to the right section I could try to resolve it... weird though it looks like some errors are coming from organization where the data is hardcoded. Maybe some repos were moved recently?

bebraw · 2017-02-05T07:12:04Z

@skipjack I merged the related PRs. Hopefully that helps with the problem. It had to do with new loaders that were added to an organization we scrape. Basically we capture issues in their readmes too now.

skipjack · 2017-02-05T21:40:09Z

@bebraw so apparently the hyperlink package fails on 301s and 302s. I was able to fix this for img.shields... links in the organization.jsx component. However, the /website links in support.jsx pretty much have to return 302s so I'm not sure what we can do to fix this... maybe we can allow 302s via some hyperlink configuration.

Aside from these the other link errors had to do with us scraping plugins/loaders. From my standpoint I think we should merge this and then debug and fix these in another PR, but I'll leave merging to you in case you feel differently.

bebraw · 2017-02-06T06:14:56Z

content/concepts/loaders.md

@@ -28,7 +28,7 @@ Secondly, configure in your `webpack.config.js` that for every `.css` file the `
 module.exports = {
  module: {
    rules: [
-      {test: /\.css$/, use: ['css-loader'](/loaders/css-loader)},
+      {test: /\.css$/, use: ['css-loader'](https://github.com/webpack/css-loader)},


Why to change this one?

That was one of the links that was failing but I guess only locally for me, now that I go to the live site I see that page does exist -- weird, does do I need to run more than just npm run build to pick up all the remote content?

There's npm run fetch for getting the remotes.

bebraw · 2017-02-06T06:16:31Z

@Munter Any thoughts on 301/302s?

Munter · 2017-02-06T07:35:37Z

302's should be valid, since there is no way to automatically conclude what the correct updated url should be for a temp redirect.

301 should be considered an error IMO. Link to the correct resource

bebraw · 2017-02-06T07:59:12Z

A couple of link notes:

http://clarkdave.net/2015/01/how-to-use-webpack-with-rails/ is gone, better remove unless you can find the article's new place.
https://github.com/webpack/webpack/tree/master/examples/component same, maybe point to the right place.
Open Collective is the trickiest as they give no guarantees on how their redirect works. It might or might not work. One fix would be to do the check on our side before injecting, but that feels a little sketchy.

skipjack · 2017-02-06T22:32:36Z

@bebraw another thing I'm wondering is why CI is executing ok on other PRs even though this seems to be a global problem.

skipjack · 2017-02-06T23:29:55Z

@bebraw and yea these two I can fix:

http://clarkdave.net/2015/01/how-to-use-webpack-with-rails/ is gone, better remove unless you can find the article's new place.

https://github.com/webpack/webpack/tree/master/examples/component same, maybe point to the right place.

SpaceK33z

Nice work again :). Noticed two thingies:

The hover for the StackOverflow / GitHub thingies is inconsistent; hovering results in a blue icon, while other items in the navbar stay white:

I think we do need to have some hover color in the navbar btw, but that isn't really in the scope of this PR.

The GitHub & StackOverflow icons don't have a title tag. That would be nice for people who don't happen to recognize the icon / screenreaders.

This reverts commit afdc323.

… with top-level links

bebraw · 2017-02-08T13:53:45Z

Before merging, we should figure out how to get around hyperlink (ideally we'll submit a PR), but I'm fine with a local hack just to get the PR in without breaking Travis.

skipjack · 2017-02-08T14:18:54Z

302's should be valid, since there is no way to automatically conclude what the correct updated url should be for a temp redirect.

@Munter The weird thing is the links that are failing do return 302s. So maybe we don't even need to PR the ignore feature, we just need a bugfix?

Here's an example of one.

Munter · 2017-02-08T23:27:42Z

I found some time. Try hyperlink v2.7.0 with --exclude https://opencollective.com/webpack/*/website

skipjack · 2017-02-09T02:32:59Z

@Munter thanks for the quick update! Just tried it out... it seems to be semi-working (I'm noticing the # SKIP log) although it still exits with a 1:

Guessing --root from input files: file:///.../Webpack/build/

  2747 tests complete (1m 5.5s)
  
  Crawling 1200 outgoing urls should respond with HTTP status 200 # SKIP
    ---
      operator: error
      expected: "200 https://opencollective.com/webpack/sponsor/10/website"
      actual:   "404 https://opencollective.com/webpack/sponsor/10/website"
      at: build/index.html:58:2217 <a class="support__item" href="https://opencollective.com/webpack/sponsor/10/website" target="_blank">...</a>
    ...
  
  Crawling 1200 outgoing urls should respond with HTTP status 200 # SKIP
    ---
      operator: error
      expected: "200 https://opencollective.com/webpack/sponsor/28/website"
      actual:   "404 https://opencollective.com/webpack/sponsor/28/website"
      at: build/index.html:58:5547 <a class="support__item" href="https://opencollective.com/webpack/sponsor/28/website" target="_blank">...</a>
    ...
  
  [ some more omitted for brevity ]


npm ERR! Darwin 16.4.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "run" "lint:links"
npm ERR! node v6.2.2
npm ERR! npm  v3.9.5
npm ERR! code ELIFECYCLE
npm ERR! [email protected] lint:links: `hyperlink -r --exclude https://opencollective.com/webpack/*/*/website build/index.html | tap-min`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] lint:links script 'hyperlink -r --exclude https://opencollective.com/webpack/*/*/website build/index.html | tap-min'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the webpack.js.org package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     hyperlink -r --exclude https://opencollective.com/webpack/*/*/website build/index.html | tap-min
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs webpack.js.org
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls webpack.js.org
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /.../Project/Webpack/npm-debug.log

Any ideas? I'll push the changes so you can see it for yourself in the travis build.

…t checking

Munter · 2017-02-09T21:40:02Z

I ran the the linting from this branch locally and can't recreate a process exiting with an error for the skipped website-urls. I do see a bit of flakiness, like these:

should respond with HTTP status 200
	operator: error
	expected: '200 https://mochajs.org/'
	actual:   'DNS Missing https://mochajs.org/'

should respond with HTTP status 200
	operator: error
	expected: '200 https://codecov.io/gh/babel/babel-loader'
	actual:   '503 https://codecov.io/gh/babel/babel-loader'

But I also had runs where the only errors in the raw tap output were the skipped opencollective website links: https://gist.github.com/Munter/8c2ead255a2abad695d39af7ba86dda4

I suspected tap-min to maybe not understand the skips, and exit with an error regardless, but that is not the case. The linting passes for me when there is no temporary dns flakiness or server outage on other external links

skipjack · 2017-02-10T15:08:11Z

@Munter yea I've noticed that flakiness as well. It does look like CI has picked up the correct version of hyperlink (v2.7.0) because those # SKIP logs are appearing there so I'm not really sure what's going on.

TBH, I'm kind of at a loss on what to do with this PR as I've never run into this situation before. It now contains a whole bunch of fixes that I'm worried will start conflicting with similar fixes on master.

I guess I can cherry pick some things to new, smaller PRs that hopefully don't fail? What's been driving me really nuts is that the nature of the errors we're seeing in CI should be representing themselves on other PRs as well, yet they aren't. I'm not familiar enough with our CI process to debug this much further, I'm not sure where else to look but hyperlink as that's what npm is saying failed out. However, if @Munter is unable to replicate, then maybe it is a problem with some other package/process.

@webpack/documentation-team any advice or assistance would be much appreciated. This is starting to hold me back from getting other things done like #409, which I started but don't want to leave this floating in the wind while I finish it as it's going to be a more ongoing task.

bebraw · 2017-02-10T15:19:50Z

It's really close. The remaining errors are like this due to Open Collective:

  Crawling 1201 outgoing urls should respond with HTTP status 200 # SKIP
    ---
      operator: error
      expected: "200 https://opencollective.com/webpack/sponsor/12/website"
      actual:   "404 https://opencollective.com/webpack/sponsor/12/website"
      at: build/index.html:58:2587 <a class="support__item" href="https://opencollective.com/webpack/sponsor/12/website" target="_blank">...</a>
    ...

Note the SKIP.

See also Munter/hyperlink#12 (comment) and Munter/hyperlink@a883caa .

Could it be that they get flagged as TAP errors?

Munter · 2017-02-10T15:52:59Z

@bebraw Those skipped errors are marked as skips in the report, see bottom of https://gist.github.com/Munter/8c2ead255a2abad695d39af7ba86dda4

Hyperlink exits with an exit code 0 if there are no errors in the report at all. However, you don't see the final report when you pipe directly to tap-min. You can recreate my results by piping the content of that gist into tap-min and see that it also doesn't exit with a non-zero code if there are no errors.

I recommend you temporarily disable the tap-min-reporter in your ci setup so we can see the full report and figure out what is happening

skipjack · 2017-02-11T14:16:52Z

@Munter @bebraw yeah so I was able to confirm that running hyperlink by itself works as we would expect. The following command:

hyperlink -r --exclude https://opencollective.com/webpack/*/*/website build/index.html

only fails when it's piped to tap-min. And it still exits with a 1 if we leave out the --exclude ... running it by itself, which is also what we'd expect. This leads me to believe that either there's some kind of bug with tap-min or the TAP format output by hyperlink somehow causes it to fail. I'm not too familiar with a TAP so I can't really say which it is. One test might be to pipe the output to other tap- packages and see how they behave.

A potential solution here should be to just remove tap-min for now. This means a messier output but would get this across the finish line and further debugging could be done in another PR.

bebraw · 2017-02-11T14:31:33Z

@skipjack Yup. Let's drop tap-min for now. Easier to resolve in another PR.

bebraw · 2017-02-11T14:33:19Z

@skipjack I don't see any skip specific handling. So it could be a problem in tap-min.

See comments in #816. `hyperlink` and `tap-min` are not playing nice together so we decided to drop `tap-min` for now and debug this issue more in another PR.

bebraw · 2017-02-11T15:08:47Z

Looks like it passed. Thanks for debugging. 👍

I'll leave merge to @skipjack.

skipjack · 2017-02-11T15:14:02Z

😝 finally!

Thanks for your help with this @Munter! @bebraw I'll leave it up to you whether we need to open an issue for debugging and re-integrating some kind of tap- output.

derhuerst · 2017-02-11T16:31:24Z

@bebraw Thanks for getting in touch with me. Will enable the issue tracker and fix the issue in tap-min asap. If you want, feel free to submit a PR.

derhuerst · 2017-02-11T16:44:46Z

FYI (not sure if you want this discussion here):

I ran the TAP log you posted above through tap-spec, faucet, tap-summary and tap-json. Only tap-json exits with 0, the others exit with non-0. This is even though the TAP homepage mentions the # SKIP directive explicitly. So i'm actually not sure what the desired behaviour is here.

Also, I cannot reproduce tap-min exiting with non-0 with the test log linked above. I get errors but exit code 0.

bebraw · 2017-02-11T16:48:38Z

@derhuerst It's fine. Thanks for pitching in and sharing your results.

The logical behavior to me would be to not take skipped tests into account when aggregating the results. It's possible this is a niche case and tap-spec, faucet, or tap-summary haven't had to deal with it. Maybe we should poke those projects too for more ideas.

derhuerst · 2017-02-11T16:54:27Z

@brebaw tap-parser, the lib that I rely on, has test cases for # SKIP. So i guess it's up to our judgement if skipped failing tests are worth reporting (and therefore failing) or not.

bebraw · 2017-02-11T17:13:11Z

@derhuerst If you want to retain the old behavior, a flag to enable skipping would be completely fine I think. 👍

Munter · 2017-02-12T16:07:50Z

I might be implementing wrong behavior in hyperlink by actually outputting a test that is both failed and skipped at the same time. Can't find any specifcs ont hat in the spec though, so this might be an edge case

derhuerst · 2017-02-12T17:21:25Z

@Munter The popular tape test runner doesn't run and therefore doesn't print skipped tests. Nevertheless the # SKIP directive is explicitly mentioned on the TAP homepage.

I found TestAnything/Specification#23 to be helpful.

skipjack added 3 commits February 4, 2017 01:22

Gitter component

e53588f

Adding a new gitter button for opening the overlay Preventing it from overlapping the footer on scroll

Removing the Sidecar and adding the Gitter component into Page

4c4792b

Adding icon links for GitHub and StackOverflow into the Navigation

926aa6e

skipjack added the UI/UX label Feb 4, 2017

Merge branch 'master' into remove-sidecar

7b1d740

skipjack added 4 commits February 5, 2017 10:47

Merge branch 'master' into remove-sidecar

0187fcc

Attempt to fix some of the build errors

e113c88

[fix] Fix for text rotator bugs

173cd6d

[fix] Fixing another broken link

afdc323

bebraw reviewed Feb 6, 2017

View reviewed changes

SpaceK33z requested changes Feb 7, 2017

View reviewed changes

skipjack added 7 commits February 7, 2017 17:26

Revert "[fix] Fixing another broken link"

1ef218f

This reverts commit afdc323.

[fix] Updating rails link in caching.md

110a9d5

[fix] Fixing the moduleextensions link in configuration/index

5232368

Removing unnecessary style declaration

eb27eee

Making navigation link hover color consistent

fa3adad

Navigation sub links: formatting and making hover behavior consistent…

5babfe9

… with top-level links

Merge branch 'master' into remove-sidecar

f70f069

Merge branch 'master' into remove-sidecar

9288148

skipjack added 2 commits February 8, 2017 21:33

Merge branch 'master' into remove-sidecar

88c75af

Updating the hyperlink package and excluding open collective from lin…

40420a2

…t checking

Merge branch 'master' into remove-sidecar

2b4845a

skipjack added 2 commits February 11, 2017 09:47

[fix] Drop tap-min

81bd739

See comments in #816. `hyperlink` and `tap-min` are not playing nice together so we decided to drop `tap-min` for now and debug this issue more in another PR.

Merge branch 'master' into remove-sidecar

24e050b

skipjack merged commit e71c7ee into master Feb 11, 2017

skipjack deleted the remove-sidecar branch February 11, 2017 15:14

skipjack mentioned this pull request Feb 11, 2017

Better visibilty for sidecar icons on smaller screens #710

Closed

montogeek mentioned this pull request Apr 23, 2019

fix unclickable links under gitter button container #2959

Merged

Remove the Sidecar and Insert Links/Buttons Elsewhere #816

Remove the Sidecar and Insert Links/Buttons Elsewhere #816

Conversation

skipjack commented Feb 4, 2017

bebraw commented Feb 4, 2017

skipjack commented Feb 4, 2017

bebraw commented Feb 4, 2017

simon04 commented Feb 4, 2017

skipjack commented Feb 5, 2017 • edited Loading

bebraw commented Feb 5, 2017

skipjack commented Feb 5, 2017 • edited Loading

bebraw Feb 6, 2017

Choose a reason for hiding this comment

skipjack Feb 6, 2017

Choose a reason for hiding this comment

bebraw Feb 7, 2017

Choose a reason for hiding this comment

bebraw commented Feb 6, 2017

Munter commented Feb 6, 2017

bebraw commented Feb 6, 2017

skipjack commented Feb 6, 2017

skipjack commented Feb 6, 2017

SpaceK33z left a comment • edited Loading

Choose a reason for hiding this comment

bebraw commented Feb 8, 2017

skipjack commented Feb 8, 2017

Munter commented Feb 8, 2017

skipjack commented Feb 9, 2017

Munter commented Feb 9, 2017

skipjack commented Feb 10, 2017 • edited Loading

bebraw commented Feb 10, 2017

Munter commented Feb 10, 2017

skipjack commented Feb 11, 2017 • edited Loading

bebraw commented Feb 11, 2017

bebraw commented Feb 11, 2017

bebraw commented Feb 11, 2017

skipjack commented Feb 11, 2017

derhuerst commented Feb 11, 2017 • edited Loading

derhuerst commented Feb 11, 2017 • edited Loading

bebraw commented Feb 11, 2017

derhuerst commented Feb 11, 2017

bebraw commented Feb 11, 2017

Munter commented Feb 12, 2017

derhuerst commented Feb 12, 2017

skipjack commented Feb 5, 2017 •

edited

Loading

skipjack commented Feb 5, 2017 •

edited

Loading

SpaceK33z left a comment •

edited

Loading

skipjack commented Feb 10, 2017 •

edited

Loading

skipjack commented Feb 11, 2017 •

edited

Loading

derhuerst commented Feb 11, 2017 •

edited

Loading

derhuerst commented Feb 11, 2017 •

edited

Loading