-
Notifications
You must be signed in to change notification settings - Fork 64
feat: release schedule #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Gonna see if I can split out the Edit: done! |
83b8460 to
0f785fa
Compare
0f785fa to
ca86397
Compare
|
|
||
| <div className="bg-white dark:bg-gray-800 rounded-xl shadow-sm border border-gray-200 dark:border-gray-700 overflow-hidden mb-6"> | ||
| <div className="p-4 border-b border-gray-200 dark:border-gray-700 bg-gray-50 dark:bg-gray-900/50 flex flex-wrap items-center gap-6"> | ||
| <div className="p-4 border-gray-200 dark:border-gray-700 bg-gray-50 dark:bg-gray-900/50 flex flex-wrap items-center gap-6"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by touchup: this bottom border was redundant. (Looking again, I think the colors might be too? But at least it doesn't show now.)
|
The release line that hasn't started yet should probably be opacity: 0.5 or something, not the same visual treatment as the active alpha line? Or maybe purple because it's nightly? |
|
@MarshallOfSound fair! I added a separate purple theme for the nightly row in 99b5972. now they match the colors on the home page! |
|
P.S. I added stripes to the nightly & prerelease rows because (1) I think they're cool and (2) they help disambiguate the green and yellow for deuteranopic color blindness (and verified this with my wonderful deuteranopic partner). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely love this.
I've wanted this data in programmatic form for years now (see #18 for an earlier attempt at just JSON output, which I later refactored on the versions-json-refactor branch). Non-blocking for this PR, but I'd want to also expose this data in the future via a /schedule.json endpoint - that's the last piece of the puzzle for putting proper dates into our Releases WG project board stable prep items. If we expose it as JSON I should be able to update the automation around templating to grab the right dates and fill them in for us.
I've left some inline comments, but a big meta one: tests? Some inspiration from the tests I was fleshing out on my branch. Namely we should test the edge cases (when alphas started, etc) and a snapshot in time of correct data would be nice so that we can change this code without risking missing subtle changes to the dates.
|
|
||
| return schedule; | ||
| }, | ||
| getKeyvCache('absolute-schedule'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish we had a way to cache this data more permanently, rather than being destined to recalculate the dates for old releases for the rest of time.
Maybe we could put the historical data into a JSON file in this repo, and this code could stop when it hits a major that's already in the committed historical data? Then it would still be dynamic, but occasionally one of us could update the historical data in the JSON file and save some CPU cycles on recalculating dates that shouldn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we expose the data in JSON via /schedule.json, we could even have a scheduled workflow that automatically opens a PR to update the historical data JSON file once a release has passed EOL and the data will no longer change. Nice little feedback loop could happen there. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a big fan of the model you're proposing over the one I implemented in this PR. We should definitely implement that system, as the schedule.json file would be very useful for other applications/systems.
| const latestRelease = group.releases[0]; | ||
|
|
||
| const entry: AbsoluteMajorReleaseSchedule = { | ||
| version: `${major}.0.0`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For discussion: do we really want the .0.0 suffix here, or just the major? Since the Node.js version shown is for the latest release in a major line, it's a bit misleading to show that Node.js version and the .0.0 version number, since that might cause confusion.
I'd be in favor of dropping the suffix and just going with the major numbers, personally. I don't think the static suffix on all of them adds any value (I know this is how we have it in the Markdown at the moment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Node.js version shown is for the *.0.0 release—unless there are no stable releases yet, in which case it uses the latest pre-release for that line.
You make a good point! Here's how I see it
- In favor of keeping the suffix: it gives each row’s major “key” a strong visual anchor and helps clarify the nature of the data.
- Against it: it adds noise and slightly constrains what we put in the table.
Not picky about which direction we take--just wanted to offer that perspective before changing it. With that context are you still leaning toward dropping the suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Node.js version shown is for the
*.0.0release—unless there are no stable releases yet, in which case it uses the latest pre-release for that line.
Ah, I misread the code, you're right. With that in mind, I'm fine leaving it with the .0.0 suffix for now.
Alternatively, we could make the suffix .x.y which would leave the visual anchor, but avoid any confusion about what info applies to the whole major line versus just the .0.0 version.
| <div className="flex items-start gap-2 text-sm text-gray-500 dark:text-gray-500"> | ||
| <Info className="w-4 h-4 mt-0.5 flex-shrink-0" /> | ||
| <p> | ||
| Release dates are goals and may be adjusted at any time for significant reasons, such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we want to have this call out higher on the page instead? Realistically, I don't think someone is going to scroll all the way down and see this organically.
An alternative could be a sort of on hover info tooltip at the top. With "Release dates are goals and may be adjusted at anytime" with the longer description shown on hover.
Now correctly predicts alpha/beta dates for most versions. Deliberately corrects some beta release dates that were incorrectly published as Wednesday instead of Tuesday.
|
@dsanders11 Sorry -- need a fresh review after the merge commit 😄 |
Description of Change
Adds a schedule that shows key dates about major Electron releases:
This information matches the manual table in the docs.
Checklist