Skip to content
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

Disable current release line when not yet cut #990

Closed
wants to merge 7 commits into from

Conversation

phillipj
Copy link
Member

@phillipj phillipj commented Oct 23, 2016

These changes makes the website handle periodes where a current release line has not been cut yet, automatically. As discussed in #982, we could grey out the "current" option, instead of two LTS options, in periodes where we don't have a current release line.

Rationale behind handling this automatically is to prevent us having to land a big PR, which we probably haven't tested much, asap after v7 has been cut. This would just work whenever v7 is out.

The main logic around this, is found in scripts/helpers/latestversion.js which picks which version should be displayed as LTS and current, plus "upcoming current" introduced with these changes. That logic is based around the list of releases in https://nodejs.org/download/release/index.json. In summary the logical rules are:

  • LTS: latest version in with .lts === true
  • Current: latest version with .lts !== true and version number greater than the latest LTS
  • Upcoming current: whenever current doesn't exists, calculate upcoming current to be major of LTS + 1, e.g. v6 + 1

Download page should probably have something similar implemented. Would appreciate any thoughts about how that might look. Tried simply greying out the background for "current" in the download matrix, but that really didn't look good, so I'm skipped that part for now.

Examples of how the frontpage looks depending on the versions found in https://nodejs.org/download/release/index.json.

Previously when LTS and current existed

screen shot 2016-10-23 at 20 06 19

#### When no current release is found (like now)

screen shot 2016-10-23 at 20 06 34

#### As soon as current exists again

screen shot 2016-10-23 at 20 07 28

## Tasks - [x] Frontpage - [x] Download page - [ ] Download page for current when current don't exist

@@ -9,9 +9,21 @@ const map = (release) => release && {
openssl: release.openssl
}

// might we undefined if the current release line has not been cut yet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: we -> be

// has not yet been cut
exports.upcomingCurrent = (releases) => {
const isCurrentReleased = exports.current(releases) !== undefined
const ltsMajor = semver.major(exports.lts(releases).node)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to exit early when isCurrentReleased is true.

@lpinca
Copy link
Member

lpinca commented Oct 23, 2016

I like the idea, 👍.

@MylesBorins
Copy link
Member

This LGTM

#992 can be closed if this lands before the v7 release.

@@ -9,9 +9,21 @@ const map = (release) => release && {
openssl: release.openssl
}

exports.lts = (releases) => {
const match = releases.find((release) => release.lts && semver.lte(release.version, '5.0.0'))
// might we undefined if the current release line has not been cut yet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'might be' instead of 'might we'

@phillipj
Copy link
Member Author

Been trying a couple of different approaches on the download page, it's quite obvious I'm no designer... What do you think of these ones? I'm more than open to other alternatives here :)

With upcoming current

screen shot 2016-10-24 at 22 19 04

Both release lines present

screen shot 2016-10-24 at 22 22 41

@ryanmurakami
Copy link
Member

@phillipj Would the "with upcoming current" grey text be disabled? I like that approach.

@phillipj
Copy link
Member Author

@phillipj Would the "with upcoming current" grey text be disabled? I like that approach.

Yepp, when grey it's just text, not an <a> at all.

@phillipj phillipj changed the title WIP: disable current release line when not yet cut Disable current release line when not yet cut Oct 24, 2016
@phillipj phillipj removed the on hold label Oct 24, 2016
@lpinca
Copy link
Member

lpinca commented Oct 24, 2016

I think it could work. I have no better ideas (I'm also not a designer).

@MylesBorins
Copy link
Member

what would be the behavior if someone visited the 'Current' url?

@phillipj
Copy link
Member Author

Pushed a fix needed for the navigation menu found on /docs/ to work without current present:

screen shot 2016-10-24 at 23 18 11

@phillipj
Copy link
Member Author

what would be the behavior if someone visited the 'Current' url?

@thealphanerd good catch, not particularly good I'm afraid :/ That page really doesn't handle current not being present -- see screenshot below.

I've gotta sleep now. Feel free to suggest how the /download/current/ page should be handled. I'll be available tomorrow from 07.00 UTC.

screen shot 2016-10-24 at 23 20 07

@phillipj
Copy link
Member Author

^^ seems like a showstopper for merging this today IMO. Especially since /download/current has been valid for almost a week, pointing to LTS v6.

Thinking out loud, two options comes to mind for the /download/current when current doesn't exist:

  1. Display it as if the latest current version actually exists (even tho it's end of life).
  2. Display an almost empty page saying Node.js currently don't have an active current release.

Afraid I won't be able to get any of the above options implemented before v7 is hopefully released today, meaning PR #992 is probably the one to be merged today.

jasnell pushed a commit that referenced this pull request Oct 25, 2016
For the release of v7.0 we should revert commit 8c53b4b

This can be trumped by #990 if it is ready for release before v7 is cut
@Fishrock123
Copy link
Member

FWIW with how this past transition went I think it is pretty safe to say we'll probably do LTS a week before the next current again next year.

@fhemberger
Copy link
Contributor

@phillipj Hi, what's the current status of this PR?

@phillipj
Copy link
Member Author

@fhemberger thanks, needed the reminder on this one 👍

So, this got stale cause I didn't have the time to do something with the /download/current URL. Thought I give option 2 from #990 (comment) a shot:

screen shot 2017-03-28 at 21 56 21

What do you think? Would displaying a more or less blank page like that be sufficient in the short period where current does not exist?

@gibfahn
Copy link
Member

gibfahn commented Mar 28, 2017

Maybe add some info about why there isn't a release line. Otherwise it looks like a "whoops, couldn't find that link" page (like we misplaced it 😁 ).

Maybe something like:

This is temporary for a week or two while the Current release line transitions into LTS.

Also if the latest LTS release was a link that would be good.

Otherwise seems reasonable to me.

@phillipj
Copy link
Member Author

More info about why there is no Current release line and linking to the LTS download page, sounds like a good idea 👍 Little unsure about the wording you suggested:

This is temporary for a week or two while the Current release line transitions into LTS.

I read that as the Current that existed, e.g. v5, would transition into LTS after a week or two, but in reality v5 had already moved into end of life IIRC.

Maybe displaying the release plan graphics would be appropriate? Something like this:

@gibfahn
Copy link
Member

gibfahn commented Mar 31, 2017

I read that as the Current that existed, e.g. v5, would transition into LTS after a week or two, but in reality v5 had already moved into end of life IIRC.

When Node 7 came out, Node 5 had already been EoL for several months right? The previous Current was Node 6, and the reason there was a gap is that Node 6 was transitioning from Current -> LTS. Agreed that the wording isn't exactly obvious for people who don't already understand the release process.

@phillipj
Copy link
Member Author

Hm, you might be right, don't remember the exact details so I looked mostly at the screenshots I took initially when opening this PR:

The most important thing is that the visitors actually understand what we're trying to communicate. Not being a native english speaker, I'm not confident I'll find something better than what already has been suggested though.

@fhemberger
Copy link
Contributor

@phillipj Hmm, it's been a while … what's the current status of this PR? What do we need to get it merged?

@phillipj
Copy link
Member Author

phillipj commented Jun 7, 2017

I've got too much outside of GitHub these days, family and buying/building a house, so I don't have time to follow this up and push it through I'm afraid.

If anyone sees value in this and want to get it merged, please go ahead and hijack the branch.

Closing this for now.

@phillipj phillipj closed this Jun 7, 2017
@fhemberger
Copy link
Contributor

@phillipj Alright, take care!

@phillipj
Copy link
Member Author

phillipj commented Jun 7, 2017

@fhemberger thanks, appreciated 👍

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

Successfully merging this pull request may close these issues.

None yet

7 participants