-
Notifications
You must be signed in to change notification settings - Fork 123
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
Create skeleton of documentation #632
Conversation
be17e47
to
564b440
Compare
I'm marking this as ready for review despite the TODOs because I think this is a stark improvement over the previous state of our documentation. The next be steps would be:
|
masonry/src/doc/01_creating_app.md
Outdated
|
||
Let's start with the `main()` function. | ||
|
||
```ignore |
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.
```ignore | |
```rust,ignore |
I'm not sure if the Github markdown renderer (or others) handles this better (i.e. syntax highlighting), but at least it has a chance to know that this is rust code.
(Same for the other ignore
here)
I also wonder, if we should design these inline examples such that they compile, so that they're always in sync and checked via CI.
But I see this may increase the (not so interesting) code in these examples.
That could probably be avoided by # code that should not be rendered
but that on the other hand has the disadvantage, to be shown in other markdown renderers (though I'd rather accept that TBH and be sure that everything is in sync).
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 also wonder, if we should design these inline examples such that they compile, so that they're always in sync and checked via CI.
You're welcome to try. It's tough to do it in a way that doesn't make the examples tedious and unreadable. (Unless you use a lot of #
marker to hide code from the reader.)
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 also interacts with #501. That is, we definitely should make these proper examples, but that work can reasonably be deferred.
Yes, that would involve using hidden harness 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.
I've scanned through the first three chapters. There are a few grammar issues, but overall this is looking really good.
I have also make a few UX suggestions. mostly for people who stumble upon this in GitHub
@@ -0,0 +1,238 @@ | |||
# Building a "To-Do List" app | |||
|
|||
**TODO - Add screenshots - see [#501](https://github.com/linebender/xilem/issues/501)** |
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.
**TODO - Add screenshots - see [#501](https://github.com/linebender/xilem/issues/501)** | |
<!-- TODO - Add screenshots - see [#501](https://github.com/linebender/xilem/issues/501) --> |
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 I'll leave it as-is. I'd rather have a visible reminder.
color: Color, | ||
} | ||
|
||
impl WidgetMut<'_, ColorRectangle> { |
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'm surprised that this works - I thought it would be a coherence break
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.
It doesn't work, hence the TODO comment. The final doc will use the free-function syntax.
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.
Looks good!
|
||
- Being hovered. | ||
- Having pointer capture. | ||
- Having local text focus. |
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.
Below, you describe 'active text focus' and 'inactive text focus'. What's 'local text focus', though?
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 it's the same. Basically there's "the widget is focused, but the window isn't", and "the widget and the window are both focused". I need to take another crack at explaining this.
c16a727
to
725b80b
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.
Other than #632 (comment), I think this is ready to land.
I've not carefully re-reviewed since my last round, but overall it's better to get this in and allow updates to be collaborative.
This is a very welcome addition. I've been watching from the peanut gallery for the longest time, waiting for something like this. Thanks a lot for taking the time, I know how hard writing documentation is. <3 |
It's so much harder than it sounds. You have to put your brain in a certain mode and pull things from it that it really doesn't want to give you by default. |
This is a very messy, very basic skeleton of what Masonry documentation will eventually look like.
Main points are:
Next steps are:
Fixes #376 and #389.