-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ability to select multiple rows #103
Comments
The selection UI will be implemented in hightable, not in hyperparam-cli. Which data structure should we use to represent the selected rows?
|
Probably 2. I like the idea of ranges, and maybe return a struct that include ranges, but ultimately probably need to return the list of rows. The main reason being that the table might be sorted, and so the selection needs to be sorting-aware. |
Note that our implementation of the sort operation already creates an array whose length is the number of rows:
So 1. might also be feasible if sort is feasible. But for datasets like https://huggingface.co/datasets/HuggingFaceFW/fineweb, not sure it makes sense (22B rows) |
Would it make sense to express the selected rows as a query, as defined in hyparam/hyparquet#56? Pros:
Note that we don't have such a column right now ( Alternative: |
I did some research on how other products (google sheets, LibreOffice, airtable) handle the ranges (click, shift+click, ctrl+click..). Every one has its own logic and behavior, no clear standard appears. Let's try to do something simple. My idea is to handle the selection in the hightable component, while hyperparam handles the actions (filter, for example) |
I'm working on selecting the rows with a pointer. But should we allow using the keyboard too? In this case, it can be a big change, because the table cells cannot be focused at all for now. |
Let's start with just by clicking, we can evaluate keyboard options later. At one point I had a branch that allowed clicking on a row number to set a selectedRow, and add it to the url hash. This is useful for sharing a deep link to a specific row. It was also intended to fix the problem where a user would double-click to go into a fullscreen cell view, but then hitting the back button would lose the table positioning. This was before the SidePanel and so it's less of an issue now, but I do think it could be a useful feature in general. And it might be a useful mechanism for keyboard navigation. + const [selectedRow, setSelectedRow] = useState<number>()
const scrollRef = useRef<HTMLDivElement>(null)
const tableRef = useRef<HTMLTableElement>(null)
+ const selectedRowRef = useRef<HTMLTableRowElement>(null)
const latestRequestRef = useRef(0)
const pendingRequest = useRef<Promise<void>>()
const pendingUpdate = useRef(false) // true if user scrolled while fetching
// total scrollable height
const scrollHeight = (data.numRows + 1) * rowHeight
@@ -125,6 +127,16 @@ export default function HyTable({ data, cacheKey, onDoubleClickCell, setError }:
}
}, [data, firstLoad, offsetTop, rows.length, scrollHeight, startIndex, setError])
+ // handle deep links from url hash
+ // TODO: update on hash change
+ useEffect(() => {
+ const row = parseInt(window.location.hash.slice(1))
+ if (!isNaN(row)) {
+ // scroll to row after rendering
+ setTimeout(() => linkToRow(row), 0)
+ }
+ }, [])
+
/**
* Validate row length
*/
@@ -134,6 +146,17 @@ export default function HyTable({ data, cacheKey, onDoubleClickCell, setError }:
}
}
+ function linkToRow(row: number) {
+ if (selectedRow === row - 1) {
+ setSelectedRow(undefined)
+ window.history.pushState(undefined, '', window.location.pathname)
+ } else {
+ setSelectedRow(row - 1)
+ window.history.pushState(undefined, '', '#' + row)
+ scrollRef.current?.scrollTo({ top: (row - 1) * rowHeight })
+ }
+ }
@@ -195,8 +219,13 @@ export default function HyTable({ data, cacheKey, onDoubleClickCell, setError }:
</tr>
))}
{rows.map((row, rowIndex) => (
- <tr key={startIndex + rowIndex} title={rowError(row, rowIndex)}>
- <td style={cornerStyle}>
+ <tr
+ className={startIndex + rowIndex === selectedRow ? styles.selectedRow : undefined}
+ id={startIndex + rowIndex === selectedRow ? 'selectedRow' : undefined}
+ key={startIndex + rowIndex}
+ ref={startIndex + rowIndex === selectedRow ? selectedRowRef : undefined}
+ title={rowError(row, rowIndex)}>
+ <td onClick={() => linkToRow(startIndex + rowIndex + 1)} style={cornerStyle}>
{(startIndex + rowIndex + 1).toLocaleString()}
</td>
+++ b/styles/Table.module.css
@@ -128,6 +127,11 @@
z-index: 15;
box-shadow: inset 0 0 4px rgba(0, 0, 0, 0.2);
}
+/* row numbers */
+.table tbody tr td:first-child {
+ cursor: pointer;
+}
+
/* mock row numbers */
.mockRowLabel {
content: "";
@@ -138,3 +142,10 @@
background: #eaeaeb;
z-index: -10;
}
+
+.selectedRow {
+ background-color: #fbf7bf;
+}
+.selectedRow td:first-child {
+ background-color: #f1edbb;
+} |
OK, cool. I reused the colors and cursor in hyparam/hightable#18. This PR only allows to select rows and dispatch the selection to the subscribers. Updating the URL would be the task of the hightable client, ie. in another PR, depending on the UX we want in hyperparam. |
In hyparam/hightable#19 I add the ability to toggle the selection for all the rows. |
In hyparam/hightable#20, I added un/selecting a range, with shift+click. |
Next steps:
|
The current implementation shows the following warning:
|
Need to be able to select a range of rows, and apply an action to them (annotation, removal, etc).
I could imagine a couple ways this could be done. Please use your best judgement and let's try things out to see how they feel. Some ideas:
Excel-style
Click on row number to select row, and start selection. Shift-click on another to select the range.
Right click menu
Right-click on row number and have dropdown menu with option "select above" rows. (long click for mobile support?)
The text was updated successfully, but these errors were encountered: