Enhancements to ScalaDoc: New Features Implemented #21436
Conversation
- Added a check in the `LanguagePickerTag` class to handle scenarios where the `languages` argument is provided but is empty. - Implemented an early return in the `render` method to prevent rendering of the language dropdown HTML when the `languages` argument is empty. - Modified the logic to check the processed `languagesArgString`, and if it is empty after trimming and processing, the method returns an empty string, avoiding the generation of an empty `<select>` element. - Added additional debug statements to trace the flow of data, particularly the raw argument, resolved variable values, and extracted language codes. - Ensured that the tag now fails silently and returns no content if the `languages` argument is missing or empty, thus avoiding potential issues with rendering an empty template. This update improves the robustness of the `LanguagePickerTag` by ensuring it doesn't output incomplete or unintended HTML, enhancing the user experience and preventing rendering errors.
|
For context, this project is to test the viability of using scaladoc to build https://docs.scala-lang.org, which does utilise more Jekyll features than currently supported - but at the same time it makes effort to simplify the UX - such as a dedicated language picker component |
| @@ -1776,7 +1776,7 @@ object Build { | |||
| bundleCSS.taskValue | |||
| ), | |||
| libraryDependencies ++= Dependencies.flexmarkDeps ++ Seq( | |||
| "nl.big-o" % "liqp" % "0.8.2", | |||
| "nl.big-o" % "liqp" % "0.9", | |||
There was a problem hiding this comment.
for context, this was necessary to support the custom syntax
| document.addEventListener("DOMContentLoaded", function () { | ||
| const tabContainers = document.querySelectorAll('.tabs'); | ||
| tabContainers.forEach(container => { | ||
| const radios = container.querySelectorAll('.tab-radio'); | ||
| const labels = container.querySelectorAll('.tab-label'); | ||
| const contents = container.querySelectorAll('.tab-content'); | ||
|
|
||
| // Hide all tab contents except the first | ||
| contents.forEach((content, index) => { | ||
| if (index !== 0) content.style.display = 'none'; | ||
| }); | ||
|
|
||
| // Check the first radio button | ||
| if (radios.length > 0) radios[0].checked = true; | ||
| contents[0].style.display = 'block'; // Ensure the first tab content is displayed | ||
|
|
||
| labels.forEach((label, index) => { | ||
| label.addEventListener('click', () => { | ||
| // Hide all tab contents | ||
| contents.forEach(content => content.style.display = 'none'); | ||
|
|
||
| // Show the clicked tab's content | ||
| contents[index].style.display = 'block'; | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
there's a bug here in that by default all tabs are hidden, and only the last tab button works, but then it shows the contents of the first tab - https://new-docs-scala-lang.vercel.app/docs/getting-started/index.html
There was a problem hiding this comment.
in fact there is an exception on line 15 (undefined is not an object, evaluating contents[0].style)
There was a problem hiding this comment.
however checking now with Chrome it works, so this bug seems to appear in Safari, but not Chrome
|
Is this PR ready for review? (I might have some free time to review it) |
| } | ||
|
|
||
| .alt-details-control:checked + .alt-details-toggle + .alt-details-detail { | ||
| position: static; |
There was a problem hiding this comment.
hard to tell if this is necessary, given the alt-details use javascript to operate now
| } | ||
|
|
||
| class TabBlock extends Block("tab") { | ||
| override def render(context: TemplateContext, nodes: LNode*): AnyRef = { |
There was a problem hiding this comment.
there was some additional unported validation logic for acceptable combinations of tab labels, but that was pretty specialised to the needs of docs.scala-lang.org - so it would be hard to see how to make that configurable (perhaps by yaml?) - so I think its acceptable to leave out for now
There was a problem hiding this comment.
something like a matcher (if one of these labels exists, then ensure that tabs are arranged in this order..)
There was a problem hiding this comment.
This should probably be moved to a page within the "internals" section https://dotty.epfl.ch/docs/internals/index.html
| override def render(context: TemplateContext, nodes: LNode*): AnyRef = { | ||
| val inputString = nodes.head.render(context).toString | ||
|
|
||
| val pattern = """(.*)for=(.*)""".r |
There was a problem hiding this comment.
this for= is supposed to be optional (i.e. we should still be able to extract the tabName if there is no for=)
| val settings = if (parsedYaml == null) { | ||
| Map("page" -> inner) ++ global | ||
| } else { | ||
| Map("page" -> parsedYaml.asScala.toMap) ++ global |
There was a problem hiding this comment.
the global keys should be filtered out of parsedYaml - which is the old logic
There was a problem hiding this comment.
i.e. remove from parsedYaml any values where the key is in globalKeys
| } | ||
|
|
||
| // Early return if the languages argument is empty | ||
| if (languagesWithEnFirst.isEmpty) { |
| // Create the dropdown HTML | ||
| val dropdown = new StringBuilder | ||
| dropdown.append( | ||
| "<select id='language-selector' name='language' onChange='handleLanguageChange(this)' style='font-size: 14px; padding: 5px; border: 1px solid #ccc; border-radius: 3px;'>" |
There was a problem hiding this comment.
its better to set up the behaviour from a JS file, rather than call a global function (see onChange=)
Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com>
| newHeaders += header | ||
| context.put(s"tab-headers-$uniqueId", newHeaders) | ||
| } | ||
| val contents = context.get(s"tab-contents-$uniqueId") |
There was a problem hiding this comment.
this is the same fix me with pattern matching (I didn't test the previous one btw, I have no idea if it would type check)
| def get[T](key: String): Option[T] = { | ||
| data.get(key).map(_.asInstanceOf[T]) | ||
| } |
There was a problem hiding this comment.
an improvement here would be to use a scala.reflect.TypeTest implicit evidence: https://www.scala-lang.org/api/3.5.0/scala/reflect/TypeTest.html - so it actually tests the value, rather than casting
|
|
||
| configFile match { | ||
| case Some(path) => | ||
| val inputStream: InputStream = Files.newInputStream(path) |
There was a problem hiding this comment.
needs to be closed at some point - I dont see anything in javadoc to suggest the load method closes it
| // languagesOpt match { | ||
| // case Some(languages) => | ||
| // for (language <- languages) { | ||
| // // Cast the key to String to avoid type mismatch errors | ||
| // val languageCode = language("code".asInstanceOf[language.K]).asInstanceOf[String] | ||
| // val languageFolderPath = Paths.get(baseDir, languageCode) | ||
| // if (!Files.exists(languageFolderPath) || !Files.isDirectory(languageFolderPath)) { | ||
| // throw new IllegalArgumentException(s"Language folder for '$languageCode' does not exist at path: $languageFolderPath") | ||
| // } | ||
| // } | ||
| // case None => | ||
| // println(s"Warning: No languages found in configuration.") | ||
| // } |
There was a problem hiding this comment.
i commented this out as it was only checking for the existence of folder, but i figured out we need a way to check the files as well
| private def convertToJava(value: Any): Object = { | ||
| value match { | ||
| case map: java.util.Map[_, _] => | ||
| val javaMap = new LinkedHashMap[String, Object]() | ||
| map.asScala.foreach { case (k, v) => javaMap.put(k.toString, convertToJava(v)) } | ||
| javaMap.asInstanceOf[Object] | ||
| case list: java.util.List[_] => | ||
| val javaList = new java.util.ArrayList[Object]() | ||
| list.asScala.foreach(v => javaList.add(convertToJava(v))) | ||
| javaList.asInstanceOf[Object] | ||
| case other => | ||
| other.asInstanceOf[Object] | ||
| } | ||
| } |
There was a problem hiding this comment.
this doesn't seem to do much other than a deep copy, I guess snakeyaml could be parsing data as an immutable map?
| mutableProperties.put("site",siteData) | ||
|
|
||
| // val parseSettings = ParseSettings.Builder().withFlavor(Flavor.JEKYLL).build().withBlock(AltDetails()) | ||
| val liqpParser = TemplateParser.Builder() |
There was a problem hiding this comment.
So at some point the connection should be made between enabling the experimental settings flag and enabling these new features
| @@ -143,6 +143,8 @@ class ScaladocSettings extends SettingGroup with AllScalaSettings: | |||
|
|
|||
| val dynamicSideMenu: Setting[Boolean] = | |||
| BooleanSetting(RootSetting, "dynamic-side-menu", "Generate side menu via JS instead of embedding it in every html file", false) | |||
| val experimentalFeatures: Setting[Boolean] = | |||
There was a problem hiding this comment.
minor: Can this be renamed to something more meaningful?
| val matchResult = pattern.findFirstMatchIn(inputString) | ||
| if (matchResult.isEmpty) { | ||
| return ("", "", "") | ||
| } | ||
|
|
||
| val id = matchResult.get.group(1) | ||
| val title = matchResult.get.group(2) | ||
| val cssClass = matchResult.get.group(3) | ||
|
|
||
| (id, title, cssClass) | ||
| } |
There was a problem hiding this comment.
style: try using a match here.
| // "styles/staticsitestyles.css", | ||
|
|
There was a problem hiding this comment.
minor: Shouldn't the new css/js files also be for staticsite only?
| } | ||
| } | ||
|
|
||
| private def readFileContent(filePath: String): Try[String] = { |
There was a problem hiding this comment.
minor: it seems that every use case of this function always rethrows this error. Can we just not catch it in the first place?
| val filePath = s"${IncludeTag.docsFolder}/$filename" | ||
| println(s"Attempting to include file: $filePath") | ||
|
|
||
| val inputData = args(1).render(context) |
There was a problem hiding this comment.
minor: should this also be able to handle more arguments?
| } | ||
|
|
||
| object IncludeTag { | ||
| @volatile private var docsFolder: String = "_docs" |
There was a problem hiding this comment.
major: this should just be a class parameter, instead of a shared global state
| } | ||
|
|
||
| object LanguagePickerTag { | ||
| @volatile private var config: Config = null |
There was a problem hiding this comment.
question: can we get rid of this shared global state too?
There was a problem hiding this comment.
you should be able to add a constructor argument to LanguagePickerTag
This pull request includes several key updates and improvements to the scaladoc done as part of GSOC 2024:
page.<key>.site.data.<variable_name>and configuration data from_config.ymlintosite.config.