Feature - Issue #170 - Update YAML Viewer#191
Conversation
richmahn
changed the title
richmahn
changed the title
|
@richmahn I really like how the http://test.door43.org:3000/richmahn/en_ta/src/master/checking/toc.yaml looks. |
ethantkoenig
left a comment
ethantkoenig
left a comment
There was a problem hiding this comment.
Depending on how large the files we're rendering are, it might be worth using a bytes.Buffer instead of string concatenation
|
|
||
| if value != nil && reflect.TypeOf(value).String() == "yaml.MapSlice" { | ||
| value = renderHorizontalHtmlTable(value.(yaml.MapSlice)) | ||
| } else if value != nil && reflect.TypeOf(value).String() == "[]interface {}" { |
There was a problem hiding this comment.
Could we use a type switch instead of manually using the reflection library for the checks on lines 43 and 45?
|
toc.yaml files are not that big. Plus that code was written months ago. |
|
@ethantkoenig Ok, got it all |
| value := mi.Value | ||
|
|
||
| if key != nil && reflect.TypeOf(key).String() == "yaml.MapSlice" { | ||
| switch reflect.TypeOf(key).String() { |
There was a problem hiding this comment.
This is not a type switch. I'd prefer a type switch to a regular switch (which is basically the same as what we originally had) to avoid having magic strings like "[]interface {}" floating around (which breaks if it's missing an easy-to-forget space).
Ditto elsewhere.
There was a problem hiding this comment.
This wasn't even something I wrote this time, so you could me asking me to change this all over Gogs if you wanted, but ok, will look into it.
There was a problem hiding this comment.
I was originally using .String() as well. For this update I just added another if. Didn't know that then warrants a whole new rewrite. Will see if I can do this without the type being written as a string...new to this.
There was a problem hiding this comment.
I can do it if you want
|
Ok, uses the type switch now. |
phillip-hopper
left a comment
phillip-hopper
left a comment
There was a problem hiding this comment.
The yaml in the demo link is not displaying correctly: http://test.door43.org:3000/richmahn/en_ta/src/master/checking/toc.yaml
|
Also, testing locally it seems to work with |
|
@phillip-hopper: Odd that this has changed since the PR was merged. Will figure out why it isn't showing the HTML . Probably due to the template settings. |
|
Oh, I guess this never was merged. Thought it had been. That means I need to put test back onto this branch. |
|
@phillip-hopper Works now, going to merge this in since Ethan already approved. http://test.door43.org:3000/richmahn/en_ta/src/master/checking/toc.yaml |
|
I've merged in and now test.dor43.org:3000 is on our 'develop' branch, for the time being (will be using it for the DCS branding when done, but should have this commit as well) |
|
You can't merge it yourself, especially when one of the reviewers still hasn't approved. |
* Changes tabular YAML view to only work with toc.yaml files * Used switch for YAML module per code review * Uses a 'type' switch as per code review
* Changes tabular YAML view to only work with toc.yaml files * Used switch for YAML module per code review * Uses a 'type' switch as per code review
* Changes tabular YAML view to only work with toc.yaml files * Used switch for YAML module per code review * Uses a 'type' switch as per code review
|
Sorry about that. Thought Ethan approved but forgot to merge. I had assumed it was already in so put the test site back on develop branch. Are we needing more than one person to approve now? |
4 participants
Simply removes rendering ALL .yaml files as tables, and just does it for toc.yaml.
For example, http://test.door43.org:3000/richmahn/en_ta/src/master/checking/toc.yaml uses this update, showing a table with clickable "link"s, and a proper rendering of the array of arrays of maps.
Where as http://test.door43.org:3000/richmahn/en_ta/src/master/manifest.yaml shows the YAML using the default view, highlighting YAML syntax.
This also fixes how the new en_ta's toc.yaml is an array of arrays which broke the production view (https://git.door43.org/Door43/en_ta/src/master/checking/toc.yaml) showing the section data as an ugly string.
It also links "link" to
<link>/01.mdwhere as before it mapped "slug" tocontent/<link>.md(old way: https://git.door43.org/Door43/en-ta-translate-vol1/src/master/toc.yaml)