Add authoring support for @TabNavigator directive #379
Conversation
ethan-kusters
force-pushed
the
tab-navigator
branch
from
c84c867
to
f4bfd86
Compare
ethan-kusters
force-pushed
the
tab-navigator
branch
from
f4bfd86
to
b00c8ed
Compare
ethan-kusters
changed the title
ethan-kusters
force-pushed
the
tab-navigator
branch
4 times, most recently
from
b0ef09d
to
b28f301
Compare
|
@swift-ci please test |
| @@ -657,6 +657,11 @@ final class RenderIndexTests: XCTestCase { | |||
| "title" : "My Article", | |||
| "icon" : "plus.svg", | |||
| "type" : "article" | |||
| }, | |||
There was a problem hiding this comment.
I think the test is now poorly named, since it tests more than CustomIcons at this point. In my mind there are two alternatives. Either we break off the tests into individual tests and instead of XCTAssertEqual on the full string, we assert on the presence of individual elements. alternatively we just rename this test to testRenderIndexGenerationWithBookLikeContent although I am not sure I like to refer to all of these as being book-like since I can see things like this new tab navigator being useful for large amounts of related screenshots in an article within conceptual documentation for an app or something.
There was a problem hiding this comment.
Ah I disagree. I think it's still only testing custom icons- if the navigator index doesn't include just regular article pages in it then something has gone really wrong.
I just had to update the test to reflect the content of the DocC catalog since I added an additional page for a different test in RenderNodeTranslatorTests but the main thing this is testing for is still the special page that customizes an icon.
There was a problem hiding this comment.
I see what you mean, I think it's fine for this review then. FTR though I think that for someone new to the code base without the context of this PR, it is less clear what the test is checking for. We should come up with a better way of testing individual render index features without having to do this all the time.
| Hello there. | ||
| } | ||
|
|
||
| @Tab("hey", bestTab: true) { |
There was a problem hiding this comment.
Not sure this kind of test belongs in here. It's more checking the directive parser than the tab navigator directive itself. I think as you already went through so much effort to make directive arguments to content so declarative IMHO there is no point checking random keys like that.
There was a problem hiding this comment.
Hm yeah that's fair. I'm asserting on the generated RenderBlockContent when an unknown directive is encountered as well below which is a little more interesting but I agree that bestTab: true is kind of useless at this point. I can remove it.
There was a problem hiding this comment.
The unknown directive case is more interesting in the sense that it maps more closely to the structure we expect from the content specifically for tab navigators.
| }.sorted() | ||
| return (line, "\(line): \(problem.diagnostic.severity) – \(problem.diagnostic.identifier)") | ||
| } | ||
| .sorted(by: \.id) |
There was a problem hiding this comment.
The double sort feels really confusing. there must be a better way of achieving this. If we just need them sorted by line number then let's just keep the second sort. If we then need ordering on other criteria let's make it explicit and do a simple, .sorted.
There was a problem hiding this comment.
Ah I'm open to suggestions. We need it sorted on the line number so that the tests are readable but then we need the sort to be stable between test runs for multiple diagnostics that appear on the same line so we need to sort on the full string as well.
I considered dropping into a full sorted but I think those are inherently much more confusing to read.
There was a problem hiding this comment.
Right now you are relying on the sorting algorithm being stable. I think that returning a tuple of (line: Int, id: String) and just doing sorted is clearer. I really think that intuitively people know that tuples are sorted element wise from "left to right". Alternatively, if readability is a concern here consider making the initial map return something very structure like (line: Int, severity: WhateverTypeThisIs, diagID: WhateverTypeThisIs) or whatever else makes sense and then making the final map in the expression something like:
.map { "\($0.line): \($0.severity) - \($0.id)" }this has the benefit that it separates figuring out the order and generating the strings, so that people can focus the logical chunks independently.
There was a problem hiding this comment.
Oh I just hadn't considered sorting the tuple itself – will that work? Agreed that's a much nicer solution if so. (I'll try it.)
There was a problem hiding this comment.
Hm yeah a (Int, String) isn't comparable so just sorted() doesn't work unfortunately. I'll implement it manually and see how it is.
There was a problem hiding this comment.
Okay I moved to a custom .sorted. Let me know what you think.
There was a problem hiding this comment.
Interestingly enough you could write the sorted as .sorted { $0 < $1 } because tuples somehow implement <` without conforming to comparable apparently. I still think your version is better though
ethan-kusters
force-pushed
the
tab-navigator
branch
from
b28f301
to
8963083
Compare
|
@swift-ci please test |
ethan-kusters
force-pushed
the
tab-navigator
branch
from
8963083
to
a5854b0
Compare
|
LGTM |
|
@swift-ci please test |
Adds a new `@TabNavigator` directive that contains `@Tab` directives-
describing a tabbed interface element.
Tab navigators allow authors to make several images or videos available
on a page without overwhelming the reader by making them all visible at
once.
`@Tab` accepts the following unnamed parameter and can contain any kind
of markup content:
- name: The name of the tab for presentation purposes.
Example:
@TabNavigator {
@tab("Powers") {
}
@tab("Exercise routines") {
}
@tab("Hats") {
}
}
`@TabNavigator` is described on the Swift forums here:
https://forums.swift.org/t/supporting-more-dynamic-content-in-swift-docc-reference-documentation/59527#tabnavigator-14
Dependencies:
- swiftlang/swift-docc-render#403
Resolves rdar://97739641
ethan-kusters
force-pushed
the
tab-navigator
branch
from
a5854b0
to
c772e4e
Compare
|
@swift-ci please test |
Bug/issue #, if applicable: rdar://97739641
Summary
Add support for the tab navigator directive as described here: https://forums.swift.org/t/supporting-more-dynamic-content-in-swift-docc-reference-documentation/59527#tabnavigator-14
Dependencies
@tabNavigatorswift-docc-render#403