-
Notifications
You must be signed in to change notification settings - Fork 26
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
fontique: Improve docs #97
Conversation
I've been writing a fontique example and this fell out of doing that ... more to come before I take it out of draft mode. Comments welcome though. |
417d495
to
66dcf1b
Compare
66dcf1b
to
d0e29e6
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.
Thanks! This looks good.
A few peripheral details about how linking works.
fontique/src/attributes.rs
Outdated
/// # See also | ||
/// | ||
/// * [`Stretch::from_percentage`] | ||
/// * [`Stretch::ratio`] |
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.
As I mentioned in linebender/kurbo#363, I don't think these cross-links to methods on the same type are helpful.
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 find them very helpful and I think that in some cases, removing them has made these changes worse. When you have something like Collection
with a variety of methods doing different things, but with some of the same terms in them, it is useful to have an indication of which are similar sets of functionality.
Instead, someone (probably not me) should write a larger doc comment for Collection
that has sections about the functionality, especially fallbacks, and then links to the right methods.
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.
And since your argument is "the Rust std lib doesn't do them", the next move is clear: Get some better cross-linking into the Rust standard lib.
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 the reason that these aren't especially helpful is that these methods are all directly next to each other.
That is, the order of the methods is part of the documentation, and that ordering places the constructors first, and all next to each other, then the corresponding interpretation methods.
fontique/src/font.rs
Outdated
/// * [`FontInfo::has_italic_axis()`] | ||
/// * [`FontInfo::has_optical_size_axis()`] | ||
/// * [`FontInfo::has_slant_axis()`] | ||
/// * [`FontInfo::has_weight_axis()`] | ||
/// * [`FontInfo::has_width_axis()`] |
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 we should instead link directly to these at the relevant item in the list above.
With possibly a sentence like:
FontInfo allows cheaply querying for the presence of these axes using the linked methods.
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've just removed them instead as I didn't like the other options. I think that's a net negative that this info is gone.
pub fn any(&self) -> bool { | ||
self.len != 0 || self.embolden || self.skew != 0 | ||
} | ||
|
||
/// Returns the variation settings that should be applied to match the | ||
/// requested attributes. | ||
/// | ||
/// When using `parley`, these can be used to create `FontVariation` |
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.
Maybe? we should link to docs.rs here?
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.
How do other crosslinks work on docs.rs? I don't particularly like the idea of absolute links to docs.rs inside the code.
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 rust-lang/rust#74481
And indeed is similar to https://linebender.org/blog/doc-include/
Ultimately, there are two options:
- link to docs.rs, and have that be non-ideal behaviour when browsing the docs locally
- don't link to docs.rs, and have users need to search for the correct type themselves
I think the former is slightly better, but I won't block on this point
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 left it as-is for someone else to revisit if they like.
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 agree somewhat with Daniel's point about cross linking within the same type. Otherwise I approve.
d0e29e6
to
6074ef2
Compare
I think all of the review stuff has been addressed. |
No description provided.