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

Need editable data-grid (or easy way to attach inputs) #493

Closed
timheuer opened this issue Jun 22, 2023 · 13 comments · Fixed by #499
Closed

Need editable data-grid (or easy way to attach inputs) #493

timheuer opened this issue Jun 22, 2023 · 13 comments · Fixed by #499
Labels
enhancement New feature or request

Comments

@timheuer
Copy link
Member

Feature request

Using the data-grid to display tabular info (obviously), but want to make it more of an editor in-line experience as well. Would like to be able to have an 'edit' mode to change cells to editable or input capture.

Expected behavior

Would expect a nice 'editable' mode, but absent that give better ways to attach more elements to a dynamically created data-grid-cell when using the rowsData approach to binding the data.

Current behavior

No ability to make data-grid editable

Use case

Richer editing on top of structured XML or CSV

Screenshots/references

2023-06-22_16-17-57

@timheuer timheuer added the enhancement New feature or request label Jun 22, 2023
@worksofliam
Copy link

worksofliam commented Jun 23, 2023

I also need this too. I'd much prefer some events that we could tag onto instead of hacking my own methods.

// Get the data grid by id
const scroller = document.getElementById("scroller");

// Make columns editable?
scroller.cellsEditable = true;

// Handle when the cell is done editing (onblur or on enter?)
scroller.onCellDone = (e) => {...};

// Set the rows of the dg.
scroller.rowsData = data.rows;

Instead, I've got this hack which works ok.

const scroller = document.getElementById("scroller");
scroller.onclick = cellClick;

function cellClick(cell) {
  const {srcElement} = cell;
  if (srcElement && srcElement.className !== "column-header") {
    const handleChange = (target) => {
      console.log(target)
      const column = target._columnDefinition;
      const originalRow = target._rowData;
      const originalValue = originalRow[column.columnDataKey];
      const newValue = target.textContent;

        console.log({newValue, column, originalRow});

      if (originalValue !== newValue) {
        console.log("Value changed");
        target._rowData[column.columnDataKey] = newValue;
      }

      srcElement.setAttribute("contenteditable", "false");

      // Unregister the events for this cell
      srcElement.onkeydown = undefined;
      srcElement.onblur = undefined;
    }

    srcElement.onkeydown = (e) => {
      if (e.code === "Enter") {
        handleChange(e.target);

        // prevent the default behaviour of return key pressed
        return false;
      }
    }

    srcElement.onblur = (e) => {
      handleChange(e.target);
    }

    srcElement.setAttribute("contenteditable", "true");
  }
}

Note that with my cellClick, arrow keys to move the cursor do not work since data-grid overrides the arrow keys to switch highlighted cell.

Video example here: https://www.youtube.com/watch?v=N-Jn8zf6KRs&feature=youtu.be

@hawkticehurst
Copy link
Member

hawkticehurst commented Jul 6, 2023

Yeah... a more batteries-included interactive data grid (along with virtualization support for that matter, #210) is totally a north star dream of mine for the toolkit. But it's, unfortunately, something that won't be happening anytime soon (if ever) just because building an interactive data grid (and getting it right) is easily one of the most time/resource-intensive components out there.

Additionally, this is actually a request that should be bubbled up to the FAST team because they own the underlying data grid implementation.

With all that said, @worksofliam's solution/workaround with the contenteditable attribute is exactly what I would have suggested so thank you for providing that!

I was going to offer some suggested improvements to the snippet yesterday (namely that you can use a custom cell-focused event that the data grid emits to create a cleaner solution), but I ended up having fun and now I have my own suggested solution/hack for this problem 😅

Here are some of the highlighted improvements:

  • Make use of the cell-focused event that data grids emit (here are the FAST API docs where I found this info btw)
  • I added more keyboard events: "enter" will now toggle between edit and non-edit, "escape" will also exit edit mode
    • Also used e.key instead of e.code because e.code ignores keyboard layouts
  • When a cell enters edit mode the text inside the cell will be highlighted automatically
  • If using TypeScript, I added type casts/annotations that improve IntelliSense immensely
  • srcElement is a deprecated property on the Event interface so I switched it to Event.target
  • I used addEventListener/removeEventListener instead of on{event} and on{event} = undefined (btw: I believe this should actually be on{event} = null) to more explicitly add and remove events
  • Use e.preventDefault(); to prevent default event behavior instead of returning false
  • Added more null/error checks (mainly suggested by TS)
import { DataGrid, DataGridCell } from "@vscode/webview-ui-toolkit";

// ... other data grid configuration code ...

initEditableDataGrid("default-grid");

function initEditableDataGrid(id: string) {
  const grid = document.getElementById(id) as DataGridCell;
  grid?.addEventListener("cell-focused", (e: Event) => {
    const cell = e.target as DataGridCell;
    // Do not continue if `cell` is undefined/null or is not a grid cell
    if (!cell || cell.role !== "gridcell") {
      return;
    }
    // Do not allow data grid header cells to be editable
    if (cell.className === "column-header") {
      return;
    }

    // Note: Need named closures in order to later use removeEventListener
    // in the handleBlurClosure function
    const handleKeydownClosure = (e: KeyboardEvent) => {
      handleKeydown(e, cell);
    };
    const handleClickClosure = () => {
      setCellEditable(cell);
    };
    const handleBlurClosure = () => {
      syncCellChanges(cell);
      unsetCellEditable(cell);
      // Remove the blur, keydown, and click event listener _only after_
      // the cell is no longer focused
      cell.removeEventListener("blur", handleBlurClosure);
      cell.removeEventListener("keydown", handleKeydownClosure);
      cell.removeEventListener("click", handleClickClosure);
    };

    cell.addEventListener("keydown", handleKeydownClosure);
    // Run the click listener once so that if a cell's text is clicked a second time the cursor will move to the given position in 
    // the string where the cell text was clicked (versus reselecting the cell text again)
    cell.addEventListener("click", handleClickClosure, { once: true });
    cell.addEventListener("blur", handleBlurClosure);
  });
}

// Handle keyboard events on a given cell
function handleKeydown(e: KeyboardEvent, cell: DataGridCell) {
  if (!cell.hasAttribute("contenteditable") || cell.getAttribute("contenteditable") === "false") {
    if (e.key === "Enter") {
      e.preventDefault();
      setCellEditable(cell);
    }
  } else {
    if (e.key === "Enter" || e.key === "Escape") {
      e.preventDefault();
      syncCellChanges(cell);
      unsetCellEditable(cell);
    }
  }
}

// Make a given cell editable
function setCellEditable(cell: DataGridCell) {
  cell.setAttribute("contenteditable", "true");
  selectCellText(cell);
}

// Make a given cell non-editable
function unsetCellEditable(cell: DataGridCell) {
  cell.setAttribute("contenteditable", "false");
  deselectCellText();
}

// Select the text of an editable cell
function selectCellText(cell: DataGridCell) {
  const selection = window.getSelection();
  if (selection) {
    const range = document.createRange();
    range.selectNodeContents(cell);
    selection.removeAllRanges();
    selection.addRange(range);
  }
}

// Deselect the text of a cell that was previously editable
function deselectCellText() {
  const selection = window.getSelection();
  if (selection) {
    selection.removeAllRanges();
  }
}

// Syncs changes made in an editable cell with the
// underlying data structure of a vscode-data-grid
function syncCellChanges(cell: DataGridCell) {
  const column = cell.columnDefinition;
  const row = cell.rowData;

  if (column && row) {
    const originalValue = row[column.columnDataKey];
    const newValue = cell.innerText;

    if (originalValue !== newValue) {
      row[column.columnDataKey] = newValue;
    }
  }
}

@hawkticehurst
Copy link
Member

Hope that helps! And, of course, please follow up with questions/comments

With that said because FAST owns the data grid implementation I'm going to close this issue and encourage you open an issue in the FAST repo requesting these changes if you'd like a proper first-party solution, hope you understand :)

@worksofliam
Copy link

@hawkticehurst thanks for this great write up. Documenting this somewhere searchable would be super nice.

@hawkticehurst
Copy link
Member

hawkticehurst commented Jul 6, 2023

Of course! And ahhh great call, I'll actually reopen this issue and consider the resolution an addition to our docs and/or samples repo with this info.

Casual heads up: Also going on vacation at the end of this week, so this addition will likely come in August.

@hawkticehurst hawkticehurst reopened this Jul 6, 2023
@timheuer
Copy link
Member Author

timheuer commented Jul 6, 2023

Agreed, thanks @hawkticehurst! I removed the select/unselect functions as they actually seemed to be getting in the way of actual edits.

@timheuer
Copy link
Member Author

timheuer commented Jul 6, 2023

I would also change from using textContent to innerText as in my experience textContent is bringing in some additional characters versus innerText is 'pure'

@hawkticehurst
Copy link
Member

I removed the select/unselect functions as they actually seemed to be getting in the way of actual edits.

Oh interesting! I found that without the text selection functionality the cursor will not automatically focus within the cell when using the enter keyboard shortcut

It still works fine with a mouse click, but again not with keyboard shortcuts. Were you able to get cursor focus when using a keyboard shortcut?

@hawkticehurst
Copy link
Member

I would also change from using textContent to innerText as in my experience textContent is bringing in some additional characters versus innerText is 'pure'

Ooh thank you! I'll update my snippet from above and make sure any docs/samples also reflect this.

@hawkticehurst
Copy link
Member

hawkticehurst commented Jul 7, 2023

I removed the select/unselect functions as they actually seemed to be getting in the way of actual edits.

Oh interesting! I found that without the text selection functionality the cursor will not automatically focus within the cell when using the enter keyboard shortcut

It still works fine with a mouse click, but again not with keyboard shortcuts. Were you able to get cursor focus when using a keyboard shortcut?

Oh never mind, I think I realize what you were talking about now. When you use a mouse to try to position the cursor at a specific place in the cell text it will always reselect the entire string, right?

If so, I believe the solution is to change that initial click event listener call to only run once:

cell.addEventListener("click", handleClickClosure, { once: true });

Now the first time you click a data grid cell the entire text string will be highlighted but if you click again it becomes a case where you can click anywhere on the string and the cursor will move to that position.

I've updated the above snippet to reflect this change too.

@timheuer
Copy link
Member Author

timheuer commented Jul 7, 2023

When you use a mouse to try to position the cursor at a specific place in the cell text it will always reselect the entire string, right?

Yep you got it -- good update.

@timheuer
Copy link
Member Author

timheuer commented Jul 7, 2023

Found another good edit and a use case I'm trying out.

contenteditable -- in my case I'm going to set to plaintext-only instead of just true -- probably a nuance for different folks, but I can only accept plain text so a good edit.

enablying newlines -- trapping the enter key is preventing allowing newlines in the edit. I'm going to play around with checking e.shiftKey as well

@hawkticehurst
Copy link
Member

Decided to actually finish this while I still had all the context in my head. Feel free to keep adding comments here with issues/ideas/improvements or (even better) open issues in the toolkit sample repo with any issues/improvements :)

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

Successfully merging a pull request may close this issue.

3 participants