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
[Shortcuts] Improve documentation #123499
Conversation
/// Base class for actions. | ||
/// Base class for an action or command to be performed. | ||
/// | ||
/// {@youtube 560 315 https://www.youtube.com/watch?v=XawP1i314WM} |
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.
What does 560 315
mean? Is that the width and height of the video? I couldn't find any documentation on the best practices to link to videos.
Should we add some guidance on the wiki? https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#documentation-dartdocs-javadocs-etc
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.
Yes, it's the width and height of the video window. And yes, it would be good to add that to the wiki.
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.
@@ -612,13 +615,13 @@ class ActionDispatcher with Diagnosticable { | |||
} | |||
} | |||
|
|||
/// A widget that establishes an [ActionDispatcher] and a map of [Intent] to |
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.
A newbie can implement shortcuts and invoke actions manually without ever using ActionDispatcher
. Thus, I've deprioritized this type.
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.
Makes sense.
a5ef334
to
2fc4906
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.
/// Base class for actions. | ||
/// Base class for an action or command to be performed. | ||
/// | ||
/// {@youtube 560 315 https://www.youtube.com/watch?v=XawP1i314WM} |
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.
Yes, it's the width and height of the video window. And yes, it would be good to add that to the wiki.
@@ -612,13 +615,13 @@ class ActionDispatcher with Diagnosticable { | |||
} | |||
} | |||
|
|||
/// A widget that establishes an [ActionDispatcher] and a map of [Intent] to |
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.
Makes sense.
@@ -868,6 +868,8 @@ class ShortcutManager with Diagnosticable, ChangeNotifier { | |||
/// This widget establishes a [ShortcutManager] to be used by its descendants | |||
/// when invoking an [Action] via a keyboard key combination that maps to an | |||
/// [Intent]. | |||
/// | |||
/// This is similar to but more powerful than the [CallbackShortcuts] widget. |
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 hint at why it's more powerful (separation of definition location for actions and intents, ability to override the action dispatcher, etc.).
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.
Added, let me know what you think!
Goals:
CallbackShortcuts
widget as newbies will likely find it easier to learn than theShortcuts
widget