Refactor usePersistField (#13775)
This PR introduces refactors and tradeoffs in the API around the events of field input. # Refactored usePersistField The hook `usePersistField` has been refactored to be used anywhere in the app, not just inside a FieldContext. This was meant to solve a bug at the beginning but now it is just used once in `RecordDetailRelationSection` outside of the context, still this is better to have this hook like that for future use cases. We also introduce `usePersistFieldFromFieldInputContext`, for an easier API inside a FieldContext. # Introduced a new `FieldInputEventContext` To remove the drill-down of events, we introduce `FieldInputEventContext`, this allows to set only once the handlers / events. In practice it allows to have an easier time maintaining the events for the many different field inputs, because it matches the pattern we already use of taking everything from a context (`FieldContext`). # Removed drill-down from FieldInput The heavy drill-down in FieldInput has been completely removed, since everything can be derived from `FieldContext` and `FieldInputEventContext`. Also there was some readonly and other specific props, but they were all drilled down from FieldContext, so it was easier to just use FieldContext where needed. # Refactored events of `MultiItemFieldInputProps` The component `MultiItemFieldInputProps` has a contrived API, here we just remove the complex part that was persisting from inside. We now only give a classic API with `onChange` and `onEscape` the rest is left to higher levels, where it should be, because this generic component shouldn't be aware of persisting things. # Extracted the parsing logic of persisted values For each input field component, we now have a clear util that was before bound to the persist call, # Tradeoff with persist times The tradeoff before was that persistField was called anywhere, before exiting the component sometimes, now it is only called by the higher levels like table or show page, which handles this abstraction. This could be challenged, however I think that having a lot of different events, and not just `handleSubmit` and `handleCancel`, convey enough meaning for the higher levels to decide what to do in each case. A `skipPersist` argument was added in events in the rare edge cases where we want to voluntarily skip persisting even with a submit or escape, but that could be challenged because we could also say that we should use cancel for that and stick to that convention. # Handling of the bug in `ActivityRichTextEditor` Initially this refactor was prioritized for solving this bug, which was very annoying for the users. But while fixing it with the new persistField hook I just understood that the problem is not just for record title cells but for anything that is open when we click on a rich text editor. The issue is described here : https://github.com/twentyhq/core-team-issues/issues/1317 So right now I just let it as is. # Stories The stories were checking that a request was sent in some cases where persist was called before a component exiting, now that persist is only called by higher-levels I just removed those tests from the stories, because that should be the responsibility of higher levels. Also a helper `getFieldInputEventContextProviderWithJestMocks` was created that exposes a context and jest mock functions for testing this new API in stories. # Miscellaneous Deactivated tui with nx by default, because it can be annoying.
This commit is contained in:
@@ -31,6 +31,7 @@ import { useUpdateOneRecord } from '@/object-record/hooks/useUpdateOneRecord';
|
||||
import { useIsRecordFieldReadOnly } from '@/object-record/record-field/hooks/read-only/useIsRecordFieldReadOnly';
|
||||
import { isInlineCellInEditModeFamilyState } from '@/object-record/record-inline-cell/states/isInlineCellInEditModeFamilyState';
|
||||
import { useRecordShowContainerData } from '@/object-record/record-show/hooks/useRecordShowContainerData';
|
||||
import { RecordTitleCellContainerType } from '@/object-record/record-title-cell/types/RecordTitleCellContainerType';
|
||||
import { getRecordFieldInputInstanceId } from '@/object-record/utils/getRecordFieldInputId';
|
||||
import { BlockEditor } from '@/ui/input/editor/components/BlockEditor';
|
||||
import { usePushFocusItemToFocusStack } from '@/ui/utilities/focus/hooks/usePushFocusItemToFocusStack';
|
||||
@@ -369,13 +370,26 @@ export const ActivityRichTextEditor = ({
|
||||
|
||||
const recordTitleCellId = getRecordFieldInputInstanceId({
|
||||
recordId: activityId,
|
||||
fieldName: labelIdentifierFieldMetadataItem?.id,
|
||||
prefix: 'activity-rich-text-editor',
|
||||
fieldName: labelIdentifierFieldMetadataItem?.name,
|
||||
// TODO: see comments below, this is a very temporary fix,
|
||||
// it won't work for the breadcrumb title input, but that's ok for now.
|
||||
prefix: RecordTitleCellContainerType.ShowPage,
|
||||
});
|
||||
|
||||
// TODO: Here instead of closing the input, as it was intially planned, we should block if there is anything open,
|
||||
// This information should be derived from the focus stack
|
||||
// The problem with this library is that it takes the focus before anything else and does not prevent the event from bubbling
|
||||
// Because of this, other events listen at the same time, and when we're in luck, the click outside gets triggered,
|
||||
// but this leaves the door open for unpredicted behavior with click handlers conflicts,
|
||||
// we recently had a bug which was deleting what the user typed and closed the right drawer if he used backspace key.
|
||||
// We could maybe use the types of components in the focus stack.
|
||||
const handleBlockEditorFocus = useRecoilCallback(
|
||||
({ snapshot }) =>
|
||||
() => {
|
||||
// TODO: Here we want to detect anything that is open to avoid conflicts with the library click event
|
||||
// that is not prevented and propagate to other click handlers in the app.
|
||||
// Because that is how we do in the app, for example with stacked dropdowns, we always close what's open before
|
||||
// letting the click being captured by a button or input that can capture it.
|
||||
const isRecordTitleCellOpen = snapshot
|
||||
.getLoadable(isInlineCellInEditModeFamilyState(recordTitleCellId))
|
||||
.getValue();
|
||||
|
||||
Reference in New Issue
Block a user