Adding Examples and documentation for List Widget #2302
Conversation
| /// label | ||
| /// } | ||
| /// ``` | ||
| /// [`im`]: https://docs.rs/druid/0.6.0/druid/im/index.html |
There was a problem hiding this comment.
This is the link I am referring to in the description. is there a way to do it like the flex crate below?
There was a problem hiding this comment.
From my tests, it seems that [`im`] alone works, but only if you build the docs with --feature=im. I wouldn't bother.
There was a problem hiding this comment.
My general takeaway is "this should be a bit shorter". I'm usually in favor of docs that assume the user knows very little and tries to explain things from the ground up, but in this case I think having this level of explanation for a single widget may be confusing; it may give the impression this widget composes differently than other widgets.
If you think there should be more documentation explaining how composition works, I'd recommend adding it to the book, the widget trait, or the documentation root.
| /// For a static sized collection of items use [`Flex`]. | ||
| /// The best data structure to use with the list widget is [`Vector`] from the [`im`] feature. | ||
| /// To include the [`im`] feature, update Cargo.toml | ||
| /// ```ignore |
There was a problem hiding this comment.
| /// ```ignore | |
| /// ```toml |
Marking the code as toml gets you better highlighting.
| /// ``` | ||
| /// # use druid::{ Data, Lens }; | ||
| /// use druid::im::Vector; | ||
| /// #[derive(Clone, Data, Lens)] | ||
| /// struct AppState { | ||
| /// list_data: Vector<String>, | ||
| /// } | ||
| /// ``` | ||
| /// ## Create initial State | ||
| /// | ||
| /// ``` | ||
| /// # use druid::{ Data, Lens}; | ||
| /// # use druid::im::Vector; | ||
| /// # #[derive(Clone, Data, Lens)] | ||
| /// # struct AppState { | ||
| /// # list_data: Vector<String>, | ||
| /// # } | ||
| /// let initial_state = AppState { | ||
| /// list_data: Vector::from( | ||
| /// vec!( | ||
| /// "one".into(), | ||
| /// "two".into(), | ||
| /// "three".into(), | ||
| /// "four".into(), | ||
| /// )), | ||
| /// }; | ||
| /// ``` | ||
| /// ## Create widgets | ||
| /// | ||
| /// ``` | ||
| /// # use druid::widget::{ Label, List }; | ||
| /// # use druid::{ Data, Lens, Widget, WidgetExt, Env}; | ||
| /// # use druid::im::Vector; | ||
| /// # #[derive(Clone, Data, Lens)] | ||
| /// # struct AppState { | ||
| /// # list_data: Vector<String>, | ||
| /// # } | ||
| /// fn root() -> impl Widget<AppState> { | ||
| /// let list = List::new(list_item) | ||
| /// .lens(AppState::list_data); | ||
| /// list | ||
| /// } | ||
| /// | ||
| /// fn list_item() -> impl Widget<String> { | ||
| /// let label = Label::new(|data: &String, _env: &Env| | ||
| /// data.clone()); | ||
| /// label | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
This whole example seems way too big to me. It's the example equivalent of an integration test, where you'd want a unit test.
I'd recommend skipping the AppState data structure and the lensing, and just using Vector<String> in your example.
| /// ## Complex widgets | ||
| /// List can be used with any complex widgets. | ||
| /// ``` | ||
| /// # use druid::widget::{ Label, List }; | ||
| /// # use druid::{ Data, Lens, Widget, WidgetExt, Env}; | ||
| /// # use druid::im::Vector; | ||
| /// #[derive(Clone, Data, Lens)] | ||
| /// struct AppState { | ||
| /// list_data: Vector<InnerState>, | ||
| /// } | ||
| /// | ||
| /// #[derive(Clone, Data, Lens)] | ||
| /// struct InnerState { | ||
| /// name: String, | ||
| /// } | ||
| /// | ||
| /// fn root() -> impl Widget<AppState> { | ||
| /// let list = List::new(list_item) | ||
| /// .lens(AppState::list_data); | ||
| /// list | ||
| /// } | ||
| /// | ||
| /// fn list_item() -> impl Widget<InnerState> { | ||
| /// let label = Label::new(|data: &InnerState, _env: &Env| | ||
| /// data.name.clone()); | ||
| /// label | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
I think you should remove that part, or at least make it way shorter. That fact that widgets can be compose doesn't need to be documented in every container widget.
| /// | ||
| /// The list widget can have a function passed in as a closure, or an abstract closure can also | ||
| /// be provided. | ||
| /// |
There was a problem hiding this comment.
This part seems superfluous. I think you can assume the vast majority of Rust users will be familiar with how closures work. (And if they aren't, this isn't the place to explain them)
| /// //This widget is the same as the two widgets above | ||
| /// //combined. | ||
| /// fn combined() -> impl Widget<AppState> { | ||
| /// let list = List::new(|| | ||
| /// Label::new(|data: &String, _env: &Env| | ||
| /// data.clone())) | ||
| /// .lens(AppState::list_data); | ||
| /// list | ||
| /// } |
There was a problem hiding this comment.
You can probably remove the combined function; the other two are descriptive enough on their own.
|
Other than that, LGTM. Even if the PR author doesn't do any of the changes I suggested, I'd still be in favor of merging the PR as-is. |
xStrom
added
docs
Hoping to resolve #803 What is the protocol for working with issues?
Also Is there a way to link the docs to
imCrate without going externally as I do. I could not find any examples of this.