Skip to content
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

[form-builder] Make inputs focusable and manage form builder focus #393

Merged
merged 1 commit into from
Nov 30, 2017

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Nov 28, 2017

I think this is finally in a state that can be released.

Summary:

  • Focus state represented as a path to the currently active input element, and is managed from a toplevel component that wraps the form builder, receives onFocus-events and tracks the current focus path on its state.
  • The focus path can be serialized and persisted/synced with e.g. the url/location hash if we want to do that later on (see https://github.com/sanity-io/sanity/blob/d24ce42fb6f61d4e5905a5c82efe3901db9790cb/packages/%40sanity/form-builder/src/sanity/focusManagers/HashFocusManager.js for an example)
  • Most of the existing input components now has a .focus() method. The responsibility of calling this method according to current focus path belongs to the FormBuilderInput-component (which is a delegate for each and every input component in the form builder).
  • As a part of this change, the FormBuilderInput can now be passed a path-prop, which is appended to each onFocus-events. We could use this path to automatically append path segments to patch events too, but that should probably be done in a separate pass as it requires a bit more thinking to ensure backwards compatibility with custom input components.
  • I also removed the Styleable HOC from DefaultTextInput and DefaultFieldtext. (I'm starting to realize that Higher Order Components should be avoided as they often introduces more problems than they solve). In this particular case, Styleable just did a simple merge of a prop anyway, so its better to leave it explicit IMO.

Remaining bits:

  • Image and file components does not implement focus(), nor do they emit onFocus for nested fields.
  • Dealing with focus in block editor is rather hackishly implemented (it tracks the focus path for its own child nodes), but a major refactoring lies ahead of us in there anyway.

None of the remaining bits are blockers, and can be fixed later on. Lets get this merged and keep it for a few days in next to smoke out new 🐛 before we release.

@rexxars
Copy link
Member

rexxars commented Nov 29, 2017

Looks good from an initial glance - will take a deeper look in a bit. Would you mind rebase this against the current next?

@bjoerge
Copy link
Member Author

bjoerge commented Nov 30, 2017

Rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants