nodejs.org icon indicating copy to clipboard operation
nodejs.org copied to clipboard

feat: introduced changelog modal on downloads

Open ovflowd opened this issue 2 years ago • 22 comments

Description

This PR introduces the support of ChangelogModals within the Download's page of the redesigned Node.js Website

Note that this PR might increase the client bundle size 🀔 we should probably check how much of an increase it is.

Validation

The "changelog button" should open a changelog with the correctly rendered MDX data.

ovflowd avatar Mar 02 '24 15:03 ovflowd

The latest updates on your projects. Learn more about Vercel for Git ↗

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 25, 2024 7:20pm

vercel[bot] avatar Mar 02 '24 15:03 vercel[bot]

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟠 78 🟢 90 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 91 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 90 🟢 100 🟢 92 🔗
/en/download 🟢 99 🟠 89 🟢 100 🟠 82 🔗
/en/blog 🟢 100 🟢 90 🟢 100 🟢 92 🔗

github-actions[bot] avatar Mar 02 '24 15:03 github-actions[bot]

Note that this version of the ChangelogModal does not include the avatars of all contributors that made a release happen. We still need to hook this into some internal utility for getting the names of all release authors.

I remember @richardlau or @targos mentioning this, but I might be remembering this incorrectly 🀔

ovflowd avatar Mar 02 '24 15:03 ovflowd

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 84%
79.29% (452/570) 76.19% (144/189) 69.91% (79/113)

Unit Test Report

Tests Skipped Failures Errors Time
90 0 :zzz: 0 :x: 0 :fire: 4.652s :stopwatch:

github-actions[bot] avatar Mar 02 '24 15:03 github-actions[bot]

I compared the page source sizes for the two versions:

With changelog modal; Source size: 193.85 KB

Beta without changelog modal; Source size: 86.65 KB


In addition, when I check with @next/bundle-analyzer, the gzipped dimensions are as follows;

Server feat/changelog-modal;

image

Server main;

image

Due to the nature of our page structure, it is not clear exactly what these parts affect. There seems to be an increase only on ../app/[locale]/[[...path]]/page.js.


Client feat/changelog-modal;

image

Client main;

image

I'm not sure exactly how we can reduce the size here, but I'll check it out as well (if it's possible) 👀

canerakdas avatar Mar 03 '24 20:03 canerakdas

I compared the page source sizes for the two versions:

With changelog modal; Source size: 193.85 KB

Beta without changelog modal; Source size: 86.65 KB

In addition, when I check with @next/bundle-analyzer, the gzipped dimensions are as follows;

Server feat/changelog-modal;

image Server `main`; image Due to the nature of our page structure, it is not clear exactly what these parts affect. There seems to be an increase only on `../app/[locale]/[[...path]]/page.js`.

Client feat/changelog-modal;

image Client `main`; image I'm not sure exactly how we can reduce the size here, but I'll check it out as well (if it's possible) 👀

If you can investigate that'd be great! It is indeed a big bundle increase, mostly (I assume) because it is also bundling the Shiki WASM and Languages and Theme on the client-side 🀔

Can you check what's the bundle difference if we removed Shiki from the ChangelogModal?

ovflowd avatar Mar 04 '24 12:03 ovflowd

Something feels weird with how the page seems to disappear entirely with no UI element to close the modal.

targos avatar Mar 05 '24 10:03 targos

Something feels weird with how the page seems to disappear entirely with no UI element to close the modal.

Indeed, I assume we need to add an X button. Hmm. @canerakdas as I mentioned before, feel free to hijack this PR :)

ovflowd avatar Mar 05 '24 10:03 ovflowd

In relation to what @targos said, I think we could make the background around the modal more transparent to see website layout.

AugustinMauroy avatar Mar 05 '24 12:03 AugustinMauroy

how will we resolve differences or deltas from the figma's vision and reality of implementation?

I think we should open a discussion/issue to find a way forward with the Figma and potential new features that won't be on this design. And also to know how to adapt the design to the real condition.

AugustinMauroy avatar Mar 08 '24 10:03 AugustinMauroy

my opinion is that feature is not worth the code, bundle size, or UX experiences. why not link to the GitHub release?

Stepping back a bit, what this is doing is trying to present the notes/lists of changes for the release to allow users of the site a way of finding out what changed or is notable about a particular release. In the redesign this is more or less covered by linking through to the blog post for the release. The blog posts are generated via scripts/release-post based on content from the changelog over in the core nodejs/node repository.

If we look at the pre-redesign (i.e. current) website:

  • https://nodejs.org/en/download neither links to the changelog nor the blog post.
  • The download buttons on the homepage have links underneath to the changelogs on GitHub. These have been problematic because for some releases the file linked to is too large for GitHub to render in the web UI (see for example https://github.com/nodejs/nodejs.org/issues/4816 -- there was also a suggestion there to link to the GitHub release but that's also had size limitations).
  • https://nodejs.org/en/about/previous-releases also links to the changelogs on GitHub.
  • None of the pages AFAICT link to the blog posts.

So the redesign is already doing something the pre-redesign site did not -- i.e. it presents the changelog information by linking to the blog posts. I think we should keep linking to the blog posts in the redesign and do not need to repeat the information by linking to the changelogs.

richardlau avatar Mar 08 '24 15:03 richardlau

Right, the intent of this PR was to solve the issue we have of changelog rendering on GitHub.

But it would only cover/handle cases for the current LTS and current releases, afaik.

Since these changelogs use the same content as from the blog posts, probably it is good enough to just have the link to the release post.

One of the other goals of this Modal, was to have a collection of GitHub avatars rendering, that showcase all the committers that made said release possible, and pretty much highlight their contributions.

I think we could still add this, but directly to the Download page, maybe below the header?

Wdyt @nodejs/nodejs-website

Regarding lack of designers, well, at this point since we do not have anyone here volunteering that works in that field, we lack the experts to assess such cases.

(Also @richardlau I pinged you here because I recall you mentioning there was a way to get a list of all committers of a release, can you point me to the right direction?)

ovflowd avatar Mar 08 '24 15:03 ovflowd

(Also @richardlau I pinged you here because I recall you mentioning there was a way to get a list of all committers of a release, can you point me to the right direction?)

👀 I don't recall that.

richardlau avatar Mar 08 '24 15:03 richardlau

One of the other goals of this Modal, was to have a collection of GitHub avatars rendering, that showcase all the committers that made said release possible, and pretty much highlight their contributions.

I think we could still add this, but directly to the Download page, maybe below the header?

@ovflowd I didn't really get your idea, you want to have an AvatarGroup on BlogHeader with list of contributor ? and set aside the modal?

AugustinMauroy avatar Mar 08 '24 16:03 AugustinMauroy

One of the other goals of this Modal, was to have a collection of GitHub avatars rendering, that showcase all the committers that made said release possible, and pretty much highlight their contributions. I think we could still add this, but directly to the Download page, maybe below the header?

@ovflowd I didn't really get your idea, you want to have an AvatarGroup on BlogHeader with list of contributor ? and set aside the modal?

No. I want AvatarGroup on Download page, that's what I wrote on my comment

ovflowd avatar Mar 08 '24 17:03 ovflowd

(Also @richardlau I pinged you here because I recall you mentioning there was a way to get a list of all committers of a release, can you point me to the right direction?)

👀 I don't recall that.

Oh, maybe I talked about that with someone else, but I recall our generated CHANGELOG (GitHub) contains the name of the commuters from their GitHub accounts, do you know where we do get that? Maybe cc @nodejs/releasers know?

ovflowd avatar Mar 08 '24 19:03 ovflowd

(Also @richardlau I pinged you here because I recall you mentioning there was a way to get a list of all committers of a release, can you point me to the right direction?)

👀 I don't recall that.

Oh, maybe I talked about that with someone else, but I recall our generated CHANGELOG (GitHub) contains the name of the commuters from their GitHub accounts, do you know where we do get that? Maybe cc @nodejs/releasers know?

changelog-maker pulls the information out of the git commits.

richardlau avatar Mar 08 '24 20:03 richardlau

While examining the size of the page here, I noticed that all releases and changelogs were added to the page because we added the changelogs to the release in reducer. To reduce the size of the page, I have removed the changelog from the release and made it accessible via next-data 🎉

Source size: 89.19 KB

canerakdas avatar Mar 10 '24 17:03 canerakdas

@canerakdas could you rebase? And @bmuenzenmeyer could you re-review?

ovflowd avatar Mar 17 '24 11:03 ovflowd

@canerakdas I believe we need to reduce blur and add a close button. Also the changelog should only open after finishing fetching the data (IMO)

ovflowd avatar Mar 17 '24 23:03 ovflowd

I think it looks better/accessible now, you can check it in the Vercel preview 👀 cc @ovflowd

Before: image

After: image

Before/Dark: image

After/Dark: image

canerakdas avatar Mar 18 '24 15:03 canerakdas

Awesome! Would it also bee too much to ask you to check into what Richard mentioned (changelog-maker pulls the information out of the git commits.) for getting the full list of GitHub usernames for the "AvatarGroup" on the modal?

ovflowd avatar Mar 18 '24 15:03 ovflowd

I believe we can merge this as it is; But we need reviews cc @bmuenzenmeyer @nodejs/nodejs-website

ovflowd avatar Mar 24 '24 12:03 ovflowd

Ah, I completely missed this, I solved it by wrapping it with a button. If we think we may encounter such situations more often, it seems like we can use Radix primitive slot

canerakdas avatar Mar 25 '24 19:03 canerakdas