Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Merge staging into master #718

Merged
merged 87 commits into from
May 17, 2020
Merged

Merge staging into master #718

merged 87 commits into from
May 17, 2020

Conversation

benhalverson
Copy link
Member

@benhalverson benhalverson commented May 15, 2020

Description

Replaces #717 to get the latest staging branch into master so we can start with a new base to continue development.

I go into this more in this issue #711
Fixed all conflicts and linting issues

@nodejs/website-redesign
@nodejs/community-committee

Related Issues

#654 #711

amiller-gh and others added 30 commits September 7, 2019 11:48
* feat: API Docs preview and version switcher
* feat: Add design tokens and style-guide page (#311)
* feat: API Docs preview and version switcher
* feat: API Page Types
* feat: Light/dark toggle in nav
 - Add API Summary section at top of page
 - Fix dark mode on learn page
 - Add all API doc sections
 - Fix API object typings
* chore: Eslint / Prettier integration.

* chore: Change ext order to overwrite eslint rules

* chore: Add build to gitignore

* chore: Eslint fixes after config change

* chore: Update yarn.lock after rebase

* chore: Add eslint-plugin-jest and extend w/ reccs

* chore: Disable no-var-requires for .js files

* chore: Remove format-check from test script

* chore: Fix eslint errors in js files

* chore: Update husky pre-push to use lint script

* chore: Create new .eslintrc config

* chore: Update lint script, add react settings to eslint config

* chore: Ignore test dir when running lint script

* chore: Add airbnb config and jest env to eslintrc

* chore: Disable react/jsx-filename-extension

* chore: Recover .prettierrc config file that was deleted

* chore: Extend eslint config with prettier/react

* chore: Fix all but a few eslint errors

* chore: Update eslint-plugin-prettier

* chore: Disable prefer-default-exports rule

* chore: Fix eslint errors after merging with staging

* chore: Fix eslint errors in konami code

* chore: Disable import/extensions and consistent-return rules

* chore: Set return type for setPage hook in click handler to void

* fix: Clean up some errors after all the merge fixing
* fix: Only disable next line for react/danger in article
* feat: Create ReleaseToggle component
* feat: Create ReleaseCards component
* chore: Add material-design placeholder icons to release cards
* feat: Create and use detectOS util function
* fix: Check if navigator is defined before using, fixes ssr
* feat: Enhanced Dark Mode Toggle Features

This adds a new `utils/DarkModeController` controller wrapped in a new `components/controls` component.
I'm not a huge fan of the current text:

"The power of JavaScript minus the browser"

This is inaccurate as Node.js is offering a ton of interfaces and
support e.g. system IO that the Browser doesn't offer. Also, while it
is not a browser I do like to think of Node.js as an active participate
in the Web Platform... the comparison here has a negative charge that I
don't think does us any benefit.
* feat: Add Footer component to layout

* test: Add Test for Footer component

* test: Updated snapshots changed by Footer component

* style: Add right margin to footer links and refactored styles

* feat: Add blur event to dropDown
Summary: Add onBlur event to dropDownButton so the user can click
anywhere outside of dropdown menu to close it

* test: Update footer snapshot

* chore: Move eslint ingore pattern from package.json
to .eslintignore

* fix: Fixed onBlur event firing even if dropdown is closed

* chore: Removed Emoji Flags from footer dropdown
        Emoji flags are not supported by all platforms, E.g. On windows

Co-authored-by: Ben Halverson <[email protected]>
* docs(contributing.md): Added details on how to contribute

#396

* docs(contributing.md): Updated the getting started section
* feat(banner): Added a new banner component

#332

* style(banner.tsx): Added additional styling

Added additional styling to match the mockup more closely

* test(snapshot): Updated snapshot for banner.tsx

* Fix topbanner dark mode (#1)

* fix: Banner styling for dark mode

* test(snapshot): Updated snapshot for banner.tsx

* style: Lint

Co-authored-by: Saleh Abdel Motaal <[email protected]>
* fix(banner.tsx): Set border property to bannerButton css to avoid border styles from browsers user agent stylesheet

* fix(banner.tsx): Set transparent as border color

* fix(banner.tsx): Update banner.tsx snapshot
@alexandrtovmach
Copy link
Contributor

alexandrtovmach commented May 17, 2020

Merging it now! Congratulations!

@alexandrtovmach alexandrtovmach merged commit 1590709 into nodejs:master May 17, 2020
@tstreamDOTh
Copy link
Contributor

Still seeing the old version on https://nodejs.dev

@alexandrtovmach
Copy link
Contributor

@tstreamDOTh
Copy link
Contributor

tstreamDOTh commented May 18, 2020

⏰Just a kind reminder that this is still not up
@alexandrtovmach @MylesBorins

@MylesBorins
Copy link
Contributor

Hey All,

I'm looking into this now, but I have to say I'm pretty disappointed to see that ya'll landed this. A couple points

  • It doesn't appear like the preview was working... I don't see the preview comments. Were they deleted
  • I had blocked the original merge for a number of reasons and was not able to review this prior to it landing this. It seems like common courtesy would have been to at least @ me prior to landing this to give me a chance to review. Instead y'all said you were going to land in 48 hours on a Friday and landed it on a Sunday... I had no chance to check it out.
  • The commit that was landed has a MASSSIVE commit (both content and message) that was in no way cleaned up. It also squashed months of work into a single massive commit that is going to be quite hard to audit / bisect if there are any issues. There are 14,604 additions and 14,080 deletions.
  • Looking quickly at the staging.nodejs.dev website I can see the company logos are still on the main page, something I had requested be removed.

I'm going to dig into the build stuff right now but I'm really upset with how this was handled.

@MylesBorins
Copy link
Contributor

I went ahead and reviewed the staging merge commit and found 3 regressions and have opened a PR to fix them. This was primarily in build tool + content

https://github.com/nodejs/nodejs.dev/issues/735

I'm unsure if there are similar regressions for styling or gatsby, but I suggest that folks do another review... seeing that there were this many issues across content I'm unconvinced there are not other regressions.... further this is not a great look for how ready this was to land. It does not appear that folks really gave this the level of review they should have for a change of this calibre.

At this point I don't think there is any reason to revert, but I'm going to review our governance and see if we need to implement changes to ensure something like this does not happen again.

@saulonunesdev
Copy link

I’m sorry you are upset and disappointed. Even though I didn’t work directly on this PR, I feel a bit of shame. There are things we could have done better in this PR and I will collect those issues in the hopes of having some sort of postmortem so we can aim to do better in the future. I hope that you recognize that a lot of work did go into this effort though and I appreciate all of the team members who are playing a role in moving this effort forward. Thanks for your Support @MylesBorins

@amiller-gh
Copy link
Member

amiller-gh commented May 18, 2020

Hey all,

I'm also quite disappointed to see that this landed so prematurely. Aside from a number of visual regressions, dev environment regressions, unapproved use of 3rd party company logos, and missing features, we also missed out on the opportunity to coordinate a larger marketing push.

I'll be in the next WR meeting and would love to help lead a discussion on https://github.com/nodejs/nodejs.dev/issues/737 (thank you @saulonunesdev). We can use it as a retrospective on where process broke down here, and how we can improve moving forward.

In addition, I think it's time to take a step back and evaluate our roadmap again. No only will that help me get back up to speed since I've been absent the past few months – but finding realignment on team goals will help to prevent this from happing in the future.

@benhalverson
Copy link
Member Author

Welcome back @amiller-gh

@Trott
Copy link
Member

Trott commented May 18, 2020

Maybe we can add a robots.txt so this site doesn't compete with the nodejs.org until it's time to graduate it?

@benhalverson
Copy link
Member Author

unapproved use of 3rd party company logos,

I don't think this is true. According to @brianwarner of the Linux foundation. Each company that was listed is a member.
This can be seen here https://www.linuxfoundation.org/membership/members/

If they're members. They've already signed the membership agreement which includes a logo consent

In the openJS slack I asked If we had permission to use the logos.
https://openjs-foundation.slack.com/?redir=%2Farchives%2FCVAMEJ4UV%2Fp1588610081004700%3Fthread_ts%3D1588188908.008400%26cid%3DCVAMEJ4UV#/

missing features

What features are those?
The PR keeps the site as a companion site to nodejs.org I asked for feedback in this issue #711 and waited a week with the issue pinned.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 18, 2020

@benhalverson PayPal and LinkedIn are not members of the foundation and were on the main page. Independent of whether we have a legal sign on on using logos it is extremely poor form to put them on a landing page without giving the companies marketing teams a heads up that they are being included.

I explicitly asked them to be removed in my block on the initial PR and did not think I needed to make it explicit in this PR as I did not expect this to land in the weekend with notice being given on Friday.

I'm going to have to step away from this repo for a couple days because I'm pretty upset tbqh, I feel that my objections were ignored and the consensus process was not followed

edit: to be clear because I noticed you linked to the LF member list. We don't have cart Blanche authority to use the logo of any of member any way we want on any website we spin up.

@bnb
Copy link
Contributor

bnb commented May 18, 2020

I don't think this is true. According to @brianwarner of the Linux foundation. Each company that was listed is a member.

The specific conversation you're referring to asks about member companies of the OpenJS Foundation. The OpenJS Foundation and Linux Foundation are not the same thing. FWIW, I would recommend reversing this PR based on this alone - it potentially exposes us to legal issues.

What features are those?

I'm not sure how this is unclear. In @MylesBorins comment (#654 (comment)), there's a pretty large list of features that are completely missing or broken.

Especially since we've been pointing people to nodejs.dev as an in-production site that we've now broken backlinks to (see a Twitter search for nodejs.dev, with many links to specific pages) to learn Node.js from (now this content exists on the Learn page), we really shouldn't be shipping an effectively broken experience outside of a sub-site.

Outside of responding directly to replies, I'd like to echo the discontent that @MylesBorins voiced. There was an explicit, hard block on a PR with feedback that was ignored. Instead of addressing the feedback and working with the author of that feedback, the PR was closed with the express intent of opening up a PR when staging was in a better state and which could be reviewed by CommComm / TSC. In opening a new PR and merging it, that hard block was ignored and non-trivial feedback was not collected.

I'd like to recommend folks look to our Membership Guidelines - which CommComm and TSC are actively held to, but are generally useful for collaboration! - for inspiration on how to move forward in this instance:

Aim to remediate first and then discuss. If other members of the team express concerns about actions, acknowledge their concerns by stopping the actions in question and then discuss within the team to come to a common agreement.

I know everyone's trying their best.

@saulonunesdev
Copy link

Hey Guys, Just a Clarification.

All the Work is based on the figma

There you can see the logo Companies and accordingly with slack discussion and the #637 issue discussion, no one raised concerns about using those logos.

@MylesBorins
Copy link
Contributor

@saulonunesdev

See #654 (comment) I explicitly blocked on this an had 1:1 conversations with both Ben and alexandr making it clear that the staging site should not land with those logos.

Digging in heels here is not making the situation better

@MylesBorins
Copy link
Contributor

MylesBorins commented May 18, 2020

I'll simplify all of the discussion here by getting to the root of the problem. I blocked the original PR as I did not feel staging was ready to land. That PR was closed in lieu of this one

I did not explicitly block this pr from landing because I trusted the team to follow our consensus seeking process, specifically that I should sign off on this or at least give some sort of signal that I agreed with this landing.

I had 1:1 conversations with more than 1 team member outlining the issues I had.

This PR was landed without those issues resolved and no attempt was made to get my review. This is a violation of the spirit of our collaboration process and undermines the governance of this repo and project.

It is not ok.

I'm not going to push for a reversion as I don't think it will accomplish much, we already fixed the issues with the logo. The concern raised by Tierney about broken links is valid though, and worth looking into.

I've had to rewrite this last paragraph multiple times... But in short this experience combined with folks defending their actions rather than remediating has me questioning my continued involvement in the nodejs.dev project, that is really not ok.

@saulonunesdev
Copy link

Thanks for explaining @MylesBorins i was not aware of all the facts. Sorry that we came to this point.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.