-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improved Context API docs #3409
Conversation
Size Comparison
|
Visit the preview URL for this PR (updated for commit 2fb5d4c): https://yew-rs--pr3409-fix-context-api-docs-iyn2cc5p.web.app (expires Sat, 30 Sep 2023 11:59:53 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
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.
Thank you very much for PR!
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.
Lot of minor comments but LGTM
|
||
/// App theme |
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.
(Unnecessary white space added. Really not important)
let ctx = use_state(|| Theme { | ||
foreground: "#000000".to_owned(), | ||
background: "#eeeeee".to_owned(), | ||
}); |
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.
Doesn't this one have the same issue than #3396?
foreground: String, | ||
background: String, |
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.
Should probably use AttrValue 😁
/// The toolbar. | ||
/// This component has access to the context |
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.
Either it's the first sentence (one line) or it's a new paragraph, in which case we need spacing
/// The toolbar. | |
/// This component has access to the context | |
/// The toolbar. | |
/// | |
/// This component has access to the context. |
/// The toolbar. | |
/// This component has access to the context | |
/// The toolbar. This component has access to the context. |
/// Button placed in `Toolbar`. | ||
/// As this component is a child of `ThemeContextProvider` in the component tree, it also has access | ||
/// to the context. |
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.
/// Button placed in `Toolbar`. | |
/// As this component is a child of `ThemeContextProvider` in the component tree, it also has access | |
/// to the context. | |
/// Button placed in `Toolbar`. | |
/// | |
/// As this component is a child of `ThemeContextProvider` in the component tree, it also has access | |
/// to the context. |
Description
use_context
to point to the actual Yew Docs page about using contexts.use_context
here and hereFixes #2943, fixes #2927
Checklist