-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Replace default dokka searchbar with new implemented in Scala.js #11021
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
@TheElectronWill Could you look at the frontend part? I mean CSS'es and DOM manipulation part (SearchbarComponent.scala file) Maybe there are some obvious optimizations or easy ways to make it look more smooth |
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 don't have much time to review in detail, but I think that a DocumentFragment should be worth it in at least one place.
a126b8b
to
15e7094
Compare
15e7094
to
625538d
Compare
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.
Scala-js part does not have any tests
scala3doc-js/src/Main.scala
Outdated
package dotty.dokka | ||
|
||
object Main extends App { | ||
def initializeSearchbar(): Unit = Searchbar() |
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.
Can't we simply call Searchbar()
instead of defining method and calling it?
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.
changed
resultsDiv.scrollTop = 0 | ||
while (resultsDiv.hasChildNodes()) resultsDiv.removeChild(resultsDiv.lastChild) | ||
val fragment = document.createDocumentFragment() | ||
result.take(100).foreach(fragment.appendChild) |
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.
magic number
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.
refactored
} | ||
loadMoreResults(result.drop(100)) | ||
|
||
private val logoClick: html.Div = |
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.
logoClick
seems a bit misleading, maybe searchIcon
is better name?
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.
changed
@@ -0,0 +1,19 @@ | |||
package dotty.dokka | |||
|
|||
enum Matchers(func: (PageEntry) => Int) extends Function1[PageEntry, Int]: |
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 think it will be more readable if func is implemented in this way:
enum Matchers extends Function1[PageEntry, Int]:
case ByName(query: String)
case ByKind(kind: String)
def apply(p: PageEntry): Int = this match
case ByName(query) =>
val nameOption = Option(p.shortName)
val acronym = p.acronym
//Edge case for empty query string
if query == "" then 1 else
val results = List(
nameOption.filter(_.contains(query.toLowerCase)).fold(-1)(_.size - query.size),
acronym.filter(_.contains(query)).fold(-1)(_.size - query.size + 1)
)
if results.forall(_ == -1) then -1 else results.filter(_ != -1).min
case ByKind(kind) =>
p.fullName.split(" ").headOption match
case world if world.equalsIgnoreCase(kind) => 1 // kind matches
case _ => -1
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 wanted to try here some Scala3 tricks, but apparently it didn't make things clearer
|
||
class SearchbarEngine(pages: List[PageEntry]) { | ||
def query(query: List[Matchers]): List[PageEntry] = { | ||
pages.map(p => p -> query.map(_(p))).filterNot(_(1).exists(_ < 0)).sortBy(_(1)).map(_(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.
this is rather hard to understand, maybe we can split it into methods or multiple lines?
Also using code like .sortBy{ case (_, measure) => measure}
helps a lot when working with traits
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.
You're right, little monster here. I've refactored this code to make it more readable.
@@ -53,8 +60,11 @@ class ScalaSearchbarDataInstaller(val ctx: DokkaContext) extends SearchbarDataIn | |||
pages.addOne(p.getName + link, PageEntry(p.getName, p.getName, link, "")) | |||
} | |||
|
|||
private def createAcronym(s: String): Option[String] = | |||
s.headOption.filter(_.isUpper).map(_ => s.filter(_.isUpper)) |
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.
- can we move this part to scalaJS?
- fuzzt search works with names starting with lowerCase like
lowerCaseName
.lowerCaseName
will be matched if user provides e.g.loCN
or "CN" orlCasNam
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.
Moved creating acronym to JS
@pikinier20 please create a followup PR with tests |
No description provided.