Issue #61 fix by adding an instruction link #176
Conversation
|
Hi @Dulya, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
Dulya
changed the title
|
pull request is to address issue #61 . |
|
Signed the CLA |
There was a problem hiding this comment.
Thanks @Dulya.
I think the other piece to this issue is making sure in the documentation for 2.5.x, 2.4.x, etc., people know how to create a project with a previous Play version. For example in the 2.5.x new application docs, the sbt new command will use the default 2.6.x branch instead of 2.5.x. (Of course we also want to encourage people to always use the latest version and make sure they know they are deliberately using an older version.)
app/views/download.scala.html
Outdated
| <th colspan="3"> | ||
| <h4 class="previousVersionHeading">@group._1</h4> | ||
| @if(group._1 > "2.2"){ | ||
| <a href="@{"https://www.playframework.com/documentation/" + group._1 + ".x/Installing"}" class="instructionLink">Instructions</a> |
There was a problem hiding this comment.
Maybe we can call this "Setup Instructions" to be more clear.
There was a problem hiding this comment.
Yeah, you're right, I forgot this included 1.x versions.
There was a problem hiding this comment.
This code also won't work if we ever have major versions of 10 or greater.
There was a problem hiding this comment.
oh yes, due to the string comparison. I'll make the necessary changes
app/views/download.scala.html
Outdated
| <tr> | ||
| <th colspan="3"> | ||
| <h4 class="previousVersionHeading">@group._1</h4> | ||
| @if(group._1 > "2.2"){ |
There was a problem hiding this comment.
I don't think we need this check. The Installing page exists and is useful in older versions too.
There was a problem hiding this comment.
yes since there's documentation for each version we can point to them. But 1.x.x modules uses "install" while 2.x.x uses "Installing" as pages. Maybe I can add a separate route in controllers/documentation/Router resolving them to the correct places.
|
Then we can add a separate column in the "Alternative downloads" section for the documentation of each version. |
|
I think it would be fine to have something like: @{if (group._1 > "1") "Installing" else "install"} |
updated installation link for older versions and used documentation router to route to the corresponding documentation page
|
@gmethvin made the requested changes |
In case of any mismatch (but won't happen)
app/views/download.scala.html
Outdated
| <a href="@{ | ||
| if(group._1.toDouble >= 1.2) { | ||
| controllers.documentation.ReverseRouter.page(None, group._1 + ".x", if(group._1.toDouble >= 2.0) "Installing" else "install") | ||
| }else if(group._1=="1.1") { |
There was a problem hiding this comment.
Actually let's just remove the cases for the older 1.x versions. I didn't realize we needed to hack the version numbers like this.
app/views/download.scala.html
Outdated
| <th colspan="3"> | ||
| <h4 class="previousVersionHeading">@group._1</h4> | ||
| <a href="@{ | ||
| if(group._1.toDouble >= 1.2) { |
There was a problem hiding this comment.
Converting to double is not a nice way to compare versions. I would write something like:
// check if version v1 >= v2
def versionAtLeast(v1: String, v2: String): Boolean = {
import math.Ordering.Implicits._
v1.split('.').toSeq > v2.split('.').toSeq
}The imported ordering lets us compare versions as lists of string numbers.
app/views/download.scala.html
Outdated
| @@ -107,7 +108,22 @@ <h3 id="older-versions">Previous releases</h3> | |||
| <table class="releases"> | |||
| @releases.previous.groupBy(_.version.slice(0, 3)).toSeq.sortBy(_._1).reverse.map { group => | |||
There was a problem hiding this comment.
I noticed the version.slice(0, 3) is technically broken too. It's really trying to get the first two version components, but it assumes version numbers are one digit. I think what we really want is something like _.version.split('.').take(2).mkString(".").
|
@gmethvin yes, found that double conversion is not a good solution so made some changes to the code as you proposed. |
added instruction link to each alternative download version headings from 2.3 upwards