search

expressjs / expressjs.com

https://expressjs.com
Other
5.23k stars 1.42k forks source link

feat: removing BLM banner #1521

Closed wesleytodd closed 1 month ago

wesleytodd commented 1 month ago

We discussed this on a meeting a while back and I think we agreed it was time to take down this banner. To be very clear to anyone seeing this, this removal is not a political statement. This project and folks who contribute to it are very much in support of the message and believe in the broader goal of an equitable justice system.

netlify[bot] commented 1 month ago

Deploy Preview for expressjscom-preview ready!

Name Link
Latest commit dab30a9f864ec99822a05c177a3569300b9d19ba
Latest deploy log https://app.netlify.com/sites/expressjscom-preview/deploys/664f666fe8501200086c1c4f
Deploy Preview https://deploy-preview-1521--expressjscom-preview.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

wesleytodd commented 1 month ago

Ah yeah I didn't check the preview link, I just removed it based on looking at the original add commit. Maybe I missed something though or maybe some other change since caused it. I can look again later today.

jonchurch commented 1 month ago

I had merged something to tweak CSS to accommodate the banner years ago. Likely that is the root of the navbar issue.

Glad to see this PR though, had been meaning to make this. Wes and I helped land this originally (TJ coded and merged it in iirc, and we both voted in support), so appropriate to be involved in removing it.

For reference, this came in in 2020 when several major sites added a banner. React kept theirs up for approx 4 months before removing it. We never came back to remove ours while express' website slumbered, the world was rocked by covid, folks were burnt out, and the US was going through a host of its own issues.

After having it up for 4 years, much longer than any other major doc site kept theirs, we can remove the banner.

wesleytodd commented 1 month ago

Not sure I ever looked at the code for the website, this is some old school layout css 🥇.

I pushed a change. Not sure if it is the exact right one, but it seemed to fix the odd spacing left over after removing the banner.

crandmck commented 1 month ago

Thanks for doing this @wesleytodd ... IMO one big issue with this banner was that it linked to (and thus promoted) one particular organization/charity. Although it is of course a worthy one, there are literally hundreds of others, and this begs the question of why we are promoting this one rather than any other one. Although I don't think this was the case, it could lead to questions of favoritism or impropriety.

Also, the change was originally made in contravention of the contribution guidelines, i.e. in a force push, not a PR: https://github.com/expressjs/expressjs.com/commit/daeac13b38ced6b354aa94f4ba6fb3c396981895 Although it was discussed in the commit, it's a bad precedent.

Anyway, LGTM.

crandmck commented 1 month ago

Hopefully no one will mind if I go ahead and land this. Since this touches quite a few files, I want to avoid potential merge conflicts with other PRs.