Conversation
|
Preview available at https://dioxuslabs.github.io/components/pr-preview/pr-176/ |
|
I forgot to run clippy.. |
|
What part of the select focus management needs to be different in select vs other components? This PR fixes the issues for select but if we can integrate those fixes into the hook it would be better so it works for the other components |
|
Since I decided to move the focus functions directly into the The options Each option in the options map has a I am not sure how this information could be made available in |
|
There is a variant of the existing focus hook here that only adds the item to the focus state's list if it isn't disabled. I imagine we could do something similar for select? |
|
The |
|
Feel free to make changes to |
|
I added that feature now directly to the These Components could also benefit from the changes made to the FocusState. They are adjusted to conform changes in this PR without any functionality changes. If this PR is merged, I will create further PRs with those changes.
Exception to that is the |
knoxfighter
force-pushed
the
select-focus-disabled
branch
from
2ac10ff to
d284d89
Compare
|
Since this is now open without any response for 2 months: |
| /// The index of the option in the list. This is used to define the focus order for keyboard navigation. | ||
| pub index: ReadSignal<usize>, | ||
| /// The tab_index of the option in the list. This is used to define the focus order for keyboard navigation. | ||
| pub tab_index: ReadSignal<usize>, |
There was a problem hiding this comment.
Tab index is a different property which is not what we are setting here. It needs to be the index within the select element. Tab index generally should never have a value other than -1 or 0 which is not the case here
| /// The focus state for the select | ||
| pub focus_state: FocusState, | ||
| /// A list of options with their states | ||
| pub(crate) options: Signal<HashMap<usize, OptionState>>, |
There was a problem hiding this comment.
The options should have sequential ids some of which may be disabled. Why do we need a hashmap here?
| } | ||
|
|
||
| /// If you don't already have a unique id, use this hook to generate one. | ||
| pub(crate) fn use_focus_unique_id() -> usize { |
There was a problem hiding this comment.
unsure why we would need a unique id for focus. the order we focus items in should be deterministic and sequential. this can return re-used ids in fullstack if the server crates the id 0, the client will not recognize that id has already been used when assigning new ids during streaming
| ctx: FocusState, | ||
| index: impl Readable<Target = usize> + Copy + 'static, | ||
| id: usize, | ||
| tab_index: impl Readable<Target = usize> + Copy + 'static, |
There was a problem hiding this comment.
both of these should be unique and it looks like you always pass the same value for id and tab_index. Do we need id?
2 participants
This contains a lot of changes and fixes for the Select component to make disabling options properly work (see #169).
focus_prev,focus_last,focus_next,focus_first, etc. functions that are independent of the ones in FocusState. This also means that i removed FocusState from the SelectContext component.I hope this code is up to your standards and the idea is something you are willing to use.