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

Scaladoc: Grouping entries in searchbar. Add hints to searchbar #14539

Merged
merged 3 commits into from
Mar 1, 2022

Conversation

pikinier20
Copy link
Contributor

No description provided.

Copy link

@MarcinAman MarcinAman left a comment

Choose a reason for hiding this comment

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

Styling comments:

  • add some horizontal padding
  • add shadow to the border to make it stand out more (right now it blends with the background)
  • show more button should... show more records, right now you have to click it a lot of times. Also this poses a little UX question: should this search also have a more expanded version to accommodate rows from "Show more"?

image

On a totally unrelated note i think that having the same color for the location on the right and title (left) makes a unreadable mess on smaller displays:
image

@@ -96,29 +84,78 @@ class SearchbarComponent(engine: SearchbarEngine, inkuireEngine: InkuireJSSearch
})
wrapper

def createKindSeparator(kind: String) =
val kindSeparator = document.createElement("div").asInstanceOf[html.Div]

Choose a reason for hiding this comment

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

pipe / tap from scala.util.chaining?

val icon = document.createElement("span").asInstanceOf[html.Span]
icon.classList.add("micon")
icon.classList.add(kind.take(2))
val name = document.createElement("span").asInstanceOf[html.Span]

Choose a reason for hiding this comment

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

do you really need those instanceOf-s? It it quite awkward
if the type is really needed then maybe it is a time for a small utility that would take the type of the element and create one

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can spare them since you operate on html.Element API

@@ -237,32 +273,50 @@ class SearchbarComponent(engine: SearchbarEngine, inkuireEngine: InkuireJSSearch
val selectedElement = resultsDiv.querySelector("[selected]")
if selectedElement != null then {
selectedElement.removeAttribute("selected")
val sibling = selectedElement.previousElementSibling
if sibling != null && sibling.classList.contains("scaladoc-searchbar-result") then {
def recur(elem: raw.Element): raw.Element = {

Choose a reason for hiding this comment

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

please name it accordingly + most of the code is duplicated lower. You can just pass a function as an argument and extract those

icon.classList.add("fa-5x")
val header = document.createElement("h1").asInstanceOf[html.Heading]
header.textContent = "A bunch of hints to make your life easier"
val listElements: ListRoot = ul(

Choose a reason for hiding this comment

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

i think this should be ol

li("Type a phrase to search members <b>by name</b> and static sites <b>by title</b>"),
li("Type abbreviations <b>cC, caCa, camCa</b> to search for <b>camelCase</b>"),
li(
"Type a function signature to search for members <b>by signature</b> using Inkuire",

Choose a reason for hiding this comment

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

i think you need a small version of some html library.
For now maybe expand utils you have above with b and a tags and change this "text"

@pikinier20 pikinier20 force-pushed the scaladoc/searchbar-grouping branch 3 times, most recently from 61ed347 to e8b03dc Compare February 25, 2022 12:24
Copy link
Contributor

@BarkingBad BarkingBad left a comment

Choose a reason for hiding this comment

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

Overall looks good, though @MarcinAman nitpicks seem valid

@pikinier20 pikinier20 force-pushed the scaladoc/searchbar-grouping branch from e8b03dc to 76d632b Compare March 1, 2022 09:50
@pikinier20 pikinier20 enabled auto-merge March 1, 2022 09:50
@pikinier20 pikinier20 merged commit 84f7250 into scala:main Mar 1, 2022
@pikinier20 pikinier20 deleted the scaladoc/searchbar-grouping branch March 1, 2022 10:59
@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 2, 2023
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.

5 participants