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

Component which can be used to edit cells inline #844

Open
Tracked by #733
stropitek opened this issue Dec 19, 2024 · 28 comments
Open
Tracked by #733

Component which can be used to edit cells inline #844

stropitek opened this issue Dec 19, 2024 · 28 comments
Assignees

Comments

@stropitek
Copy link
Contributor

stropitek commented Dec 19, 2024

Integrate a simple input / text area within cells of a table which looks and behaves like inline editable cell.

  • Should be styled in a way that we don't really identify it's a textarea when it is not focused.
  • Should grow as the user types in characters, and stop growing once the size of the cell is reached.

Tab should focus the next editable cell.

Let's start by creating a story for this and later see if it would be worth making a component out of this.

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Dec 19, 2024

I started thinking about this. I think, we can create a styled input like that :

const Input = styled.input`
  width: 100%;
  background-color: inherit;

  :hover,
  :focus {
    border: solid black 1px;
  }
`;

The input will take 100% width. Bad things, he will not grow with the text but, cannot take a larger width than parent div.
With this stories it can look like that :

export function Test() {
  const [state, setState] = useState('');

  return (
    <div>
      <div style={{ backgroundColor: 'red', maxWidth: 200 }}>
        <Input onBlur={(event) => setState(event.currentTarget.value)} />
      </div>

      <div>{state}</div>
    </div>
  );
}

CleanShot 2024-12-19 at 13 52 16

Do we need something like that ? If yes, we do not have to create a new Input it looks ok !

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Jan 3, 2025

With our latest meeting we said that :

Ajouter une story avec un tableau qui contient des cellule éditable et le maximum de fonctionnalité pour ce tableau.
L'input pour l'édition doit avoir une width à 100%, et comme max-width la taille maximale de la cellule. Comme ça elle prend toute la taille disponible.

On doit ajouter une action afin de supprimer une ligne (icon sur la droite)
Permettre l'ajout de ligne rapide dans un tfooter (regarder pour le colSpan)

Pour l'édition inline :

  • hover & focus ajout de l'outline (icon d'édition en petit à droite ? - idée non validé)
  • input prend 100% taille de la cellule
  • onFocus (ou onClick) doit transformer la cellule du tableau en input ou en select
  • Attention de mettre tabIndex à zéro sur l'élément (tester tout ça avec plusieurs tableau dans une story et essayer de se déplacer avec tabulation)
  • input en autoFocus

Créer un composant utilisable n'importe ou qui peux faire un rendu conditionnelle.

  • Mode focus (affiche l'input ou le select)
  • Mode non-focus qui affiche la cellule (juste la valeur)
  • Par défaut le mode de base doit être "non-focus"

On admet que pour sélectionner une ligne on doit avoir un radio ou une checkbox

Voici la liste des composants possible pour faire le rendu :

  • input
  • input & datalist
  • select

Petit plus :

  • Ajouter un petit oeil pour cacher / rendre visible l'élément dans la column tout à gauche (possiblement à la place de l'id, ou à côté)

@Sebastien-Ahkrin
Copy link
Contributor

With theotime, we've been thinking about a proposal for input (and only input for the moment)
It may look like this:

import type { ReactNode } from 'react';

export interface InlineRendererEditableProps<T extends HTMLElement> {
  isRendered: boolean;
  ref: (node: T | null) => void;
  onChange: (value: string) => void;
}

export interface InlineEditableRendererProps<T extends HTMLElement> {
  renderEditable: (props: InlineRendererEditableProps<T>) => ReactNode;
  children: ReactNode;
}

export function InlineEditableRenderer<T extends HTMLElement>(
  props: InlineEditableRendererProps<T>,
) {
  return null;
}

export function InputTest() {
  return (
    <InlineEditableRenderer
      renderEditable={({ onChange, ...props }) => (
        <input
          {...props}
          onBlur={(event) => {
            onChange(event.currentTarget.value);
          }}
        />
      )}
    >
      Input
    </InlineEditableRenderer>
  );
}

InputTest shows how it would be used!
Does that sound complete, or have we missed something?

@tpoisseau
Copy link

export interface InlineRendererEditableProps<Element extends HTMLElement, Value> {
  // ...
  onChange: (value: Value, preventSwitch?: boolean) => void;
}

@targos
Copy link
Member

targos commented Jan 9, 2025

Can you add state management to the example usage (with a simple useState)?

@Sebastien-Ahkrin
Copy link
Contributor

Can you add state management to the example usage (with a simple useState)?

Sure @targos !
With useState the code can look like that :

import { useState } from 'react';
import type { ReactNode } from 'react';

export interface InlineRendererEditableProps<T extends HTMLElement> {
  isRendered: boolean;
  ref: (node: T | null) => void;
  onChange: (value: string, preventSwitch?: boolean) => string;
}

export interface InlineEditableRendererProps<T extends HTMLElement> {
  renderEditable: (props: InlineRendererEditableProps<T>) => ReactNode;
  children: ReactNode;
}

export function InlineEditableRenderer<T extends HTMLElement>(
  props: InlineEditableRendererProps<T>,
) {
  return null;
}

export function InputTest() {
  const [state, setState] = useState('Input');

  return (
    <InlineEditableRenderer
      renderEditable={({ onChange, ...props }) => (
        <input
          {...props}
          defaultValue={state}
          onBlur={(event) => {
            const value = onChange(event.currentTarget.value);
            setState(value);
          }}
        />
      )}
    >
      {state}
    </InlineEditableRenderer>
  );
}

I changed the code to add preventSwitch, and now "onChange" return the value as string. this can help the developer to set the state. but if wanted, he can use event.currentTarget.value too. onChange will return this value too.

Is that ok ?

@tpoisseau
Copy link

tpoisseau commented Jan 9, 2025

In fact, InlineEditableRenderer don't need to obtain new value with onChange from renderEditable props.

renderProps props type could be:

export interface InlineRendererEditableProps<Element extends HTMLElement, Value> {
  isRendered: boolean;
  ref: (node: T | null) => void;
  toggle: () => void;
}

@targos
Copy link
Member

targos commented Jan 9, 2025

The example looks wrong. You spread InlineRendererEditableProps inside the <input> but it contains invalid props.

@tpoisseau
Copy link

I agree props should not be spread inside input.

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Jan 9, 2025

yes my bad !
I think we will have to export some custom component for input, select etc to handle the isRendered props correctly. With that the user will not have to think about the style of the input

it can looks like that then :

import type { ReactNode } from 'react';
import { useState } from 'react';

export interface InlineRendererEditableProps<T extends HTMLElement> {
  isRendered: boolean;
  ref: (node: T | null) => void;
  toggle: () => string;
}

export interface InlineEditableRendererProps<T extends HTMLElement> {
  renderEditable: (props: InlineRendererEditableProps<T>) => ReactNode;
  children: ReactNode;
}

export function InlineEditableRenderer<T extends HTMLElement>(
  props: InlineEditableRendererProps<T>,
) {
  return null;
}

export function InputTest() {
  const [state, setState] = useState('Input');

  return (
    <InlineEditableRenderer
      renderEditable={({ toggle, isRendered, ref }) => (
        <input
          ref={ref}
          defaultValue={state}
          onBlur={(event) => {
            toggle();
            setState(event.currentTarget.value);
          }}
        />
      )}
    >
      {state}
    </InlineEditableRenderer>
  );
}

@targos
Copy link
Member

targos commented Jan 9, 2025

In what case can renderEditable be called with isRendered:false ?

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Jan 9, 2025

In what case can renderEditable be called with isRendered:false ?

This props is used to add "visibility" to input. isRendered is false, when the value (children) is rendered. true, when the input (in this example) will be rendered.

MB the example is not good enough, i will add the style inside too, to make this more readable :
This is what i mean by "I think we will have to export some custom component for input, select etc to handle the isRendered props correctly. With that the user will not have to think about the style of the input" the input should be an exported component.

<InlineEditableRenderer
      renderEditable={({ toggle, isRendered, ref }) => (
        <input
          ref={ref}
          defaultValue={state}
          style={{
            visibility: isRendered ? 'visible' : 'hidden',
          }}
          onBlur={(event) => {
            toggle();
            setState(event.currentTarget.value);
          }}
        />
      )}
    >
      {state}
    </InlineEditableRenderer>

@targos
Copy link
Member

targos commented Jan 9, 2025

But you should not render the input when the component is not in edit mode. You should render the children, no?

@targos
Copy link
Member

targos commented Jan 9, 2025

Or we add another render prop, but renderEditable should only be called in edit mode.

@tpoisseau
Copy link

I remember @Sebastien-Ahkrin need to render input with visibility hidden to avoid layout shifting in table. Like that it's always in DOM, but visible only when it's active.

Maybe the default behavior should be call renderEditable only in edit mode, but have a props flag to always render it.

@targos
Copy link
Member

targos commented Jan 9, 2025

Another solution is to put visibility: hidden internally on the element that wraps it

@tpoisseau
Copy link

visibility: hidden is a "hack" for a specific use case (InlineEditableRenderer in a flexible layout) I don't think it should be use in every case. Even if now it's the only usage of InlineEditableRenderer we will have.

@Sebastien-Ahkrin
Copy link
Contributor

Another solution is to put visibility: hidden internally on the element that wraps it

yep ! its the best thing i think, because i have to render children everytime. (the editable is only render in front of the children)

@targos
Copy link
Member

targos commented Jan 9, 2025

The only use case of this component is to be put somewhere inline (it's even in the name). We always want it to have a stable size I thikn.

@Sebastien-Ahkrin
Copy link
Contributor

The only use case of this component is to be put somewhere inline (it's even in the name). We always want it to have a stable size I thikn.

then are we ok with everything ?

@targos
Copy link
Member

targos commented Jan 9, 2025

We are good to make a poc

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Jan 10, 2025

@targos we have a testable POC here: https://feat-editable-table.react-science.pages.dev/stories/?path=/story/components-editabletabletext--inline-editable

Here is the code for this story :

export function InlineEditable() {
  const [state, setState] = useState('value');

  return (
    <div>
      <span>State: {state}</span>
      <InlineEditableRenderer
        renderEditable={({ ref, toggle }) => (
          <SwitchableInput
            ref={ref}
            defaultValue={state}
            onBlur={(event) => {
              toggle();

              setState(event.currentTarget.value);
            }}
          />
        )}
      >
        {state}
      </InlineEditableRenderer>
    </div>
  );
}

@targos
Copy link
Member

targos commented Jan 10, 2025

@stropitek Can you have a look?

@Sebastien-Ahkrin
Copy link
Contributor

Sebastien-Ahkrin commented Jan 10, 2025

I just found a "bug", when the main container (div on my example) has a specific width size (like 50).
The input will no take the entire place.

like this :
Image

For explanation, the first 3 lines are "State: Hello, World!" its only a div with text (based on the state).
The first bordered text is the input "Hello, World!"
And the "World" is the text behind the input.

Do the concept of an input with a little size like this, is possible ?

This is now fixed!

@stropitek
Copy link
Contributor Author

stropitek commented Jan 13, 2025

Can SwitchableInput make sense as a standalone component, or only when used with InlineEditableRenderer? Seem to me like it does not, so I would try to find a way to make it clear that they are part of the same family of components.

Otherwise API LGTM.

@Sebastien-Ahkrin
Copy link
Contributor

Can SwitchableInput make sense as a standalone component, or only when used with InlineEditableRenderer? Seem to me like it does not, so I would try to find a way to make it clear that they are part of the same family of components.

Otherwise API LGTM.

@stropitek I renamed the component to be : BorderlessEditableInput. We can use it everywhere BUT, we have to remember that this input was created for the Switchable context. It works with position relative etc. its not a basic input.
Here is an example on how to use it :

export function BorderlessEditableInputExample() {
  return (
    <div
      style={{
        backgroundColor: 'lightblue',
        height: 25,
        width: 100,
        position: 'relative',
      }}
    >
      <BorderlessEditableInput defaultValue="Hello, World!" />
    </div>
  );
}

Its the only way to use it (container with position: relative). We do this, because blueprint create a row with td who's have position: relative. and we use position on the Switchable to display text or input.

Is this ok for you ?

@stropitek
Copy link
Contributor Author

It's specifically for those reasons that I don't want the two components to have unrelated names.

The name you suggested seems a bit specific ("borderless") and still not that much related to the InlineEditableRenderer.

Maybe change InlineEditableRenderer to InlineEditable and call the other component InlineEditableInput?

@Sebastien-Ahkrin
Copy link
Contributor

Ok ! i changed the component name. everything can be found here : #846

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

No branches or pull requests

4 participants