Refactor Focus page #10749
Refactor Focus page #10749
Conversation
|
Visit the preview URL for this PR (updated for commit 4d1b3ad): https://flutter-docs-prod--pr10749-fix-10168-copy-qvo61uju.web.app |
There was a problem hiding this comment.
I've finished reviewing about half of this page. I have concerns about the new structure/organization. I don't feel that it really makes the subject clearer.
@parlough, you filed this issue originally, was this the sort of thing you had in mind? I thought you meant that you might like more code examples, but I am not seeing that here. Please clarify.
| * To change focus on touch interfaces, | ||
| app users tap or click on the desired UI element. |
There was a problem hiding this comment.
It's ok up to here. But, in the overview, I think it's odd that you mention that the type of focus input depends on the input method, but then ONLY touch input is mentioned. I'd expect the overview to succinctly list all known types of input.
| * To change focus on traditional interfaces, | ||
| app users can use the mouse to click on the desired UI element | ||
| or press a keyboard shortcut to move focus. | ||
|
|
||
| Some examples of situations where you might need to know how to use the focus | ||
| system: | ||
| Once in focus, text entered with the keyboard flows to that element. | ||
| Text input could control objects or display text in a text field. | ||
| This continues until the focus moves to another element in the app. |
There was a problem hiding this comment.
This section doesn't list focus use cases. It only lists the traditional keyboard-based focus.
Also, it's incorrect to say "click on", the word lists says to use merely "click":
https://developers.google.com/style/word-list#click
I'd suggest removing this header (it's super short anyway, and not complete) and just finish the overview with both types of input.
| - [Changing or defining the "tab order" of focus traversal in an application](#focustraversalpolicy) | ||
| - [Defining groups of controls that should be traversed together](#focustraversalgroup-widget) | ||
| - [Preventing some controls in an application from being focusable](#controlling-what-gets-focus) | ||
| :::secondary What you'll learn |
There was a problem hiding this comment.
Why did you replace the list of focus use cases with "what you'll learn"? This isn't a tutorial. I put those use cases there for a VERY SPECIFIC reason. I wanted to make it very clear what situations might require fiddling with the focus. This new "what you'll learn" section doesn't convey the same info. And since we've already said that this guide doesn't apply to everyone, why treat it like a general tutorial?
There was a problem hiding this comment.
The list of use cases that Greg added in his initial PR read as lacking context, so they were moved to When to use the focus system.
| Which node Flutter chooses depends upon the `disposition` argument | ||
| you give to `unfocus()`. | ||
| When calling `unfocus()`, you can set the `disposition` argument. | ||
| It allows two modes for passing focus: |
There was a problem hiding this comment.
This text is verbose and duplicative. Why not make it more concise, such as:
| Which node Flutter chooses depends upon the `disposition` argument | |
| you give to `unfocus()`. | |
| When calling `unfocus()`, you can set the `disposition` argument. | |
| It allows two modes for passing focus: | |
| Flutter chooses a new focus node depending on the `disposition` argument | |
| you pass to `unfocus()`: |
| * [`UnfocusDisposition.scope`][]: | ||
| The argument defaults to this option. | ||
| It gives the focus to the nearest parent focus scope. | ||
| If then something moves the focus to the next node with `FocusNode.nextFocus`, | ||
| the app starts with the "first" focusable item in the scope. |
There was a problem hiding this comment.
Again, I'd like to see this more concise:
| * [`UnfocusDisposition.scope`][]: | |
| The argument defaults to this option. | |
| It gives the focus to the nearest parent focus scope. | |
| If then something moves the focus to the next node with `FocusNode.nextFocus`, | |
| the app starts with the "first" focusable item in the scope. | |
| * [`UnfocusDisposition.scope`][] (default): | |
| Flutter passes focus to the nearest parent focus scope. | |
| After this, calling `FocusNode.nextFocus()` moves focus to the first | |
| focusable widget in that scope. |
| ## Focus widget | ||
| * Attaches and detaches the focus node it owns from the focus tree | ||
| * Manages the attributes and callbacks of the focus node | ||
| * Enables discovery of focus nodes attached to the widget | ||
| tree through its static functions |
There was a problem hiding this comment.
Why did you replace this text with a bulleted list? Particularly following a header, it should prose, not a list. It's much harder to parse this way.
There was a problem hiding this comment.
It starts with prose and goes to a bulleted list.
|
Also, I tried viewing the staged version of this page and it was no longer available. I'd like to see that, once you do some restructuring. |
|
/gcbrun |
|
I'm going to close this for now. This change will likely be picked up again at a later date, with attribution back to this PR. |
Fixes #10168
Tech review approved in #10432