Add sort buttons (▲/▼) to TreeView columns #4265
Conversation
|
The relevant (and not already obsolete) comments from the PR #4203:
|
|
Changes:
|
|
Thanks, I find the UI much better than in the previous iteration. There's still a small weird behavior when switching columns, it looks like the column "remembers" the previous state, so for example we click on column A (it's ascending), then column B (it's ascending), then column A again (it's descending!! but I expect ascending in this case). Otherwise about the sorting implementation itself, we discussed this a bit with Markus, and we agreed that the sorting doesn't happen at the right position with this PR. This shouldn't be in the TreeView component but in the Tree structure that governs how the data is displayed. For example, this is where it should be for the marker table: .And for the call tree (if we want to do it there): profiler/src/profile-logic/call-tree.js Lines 152 to 156 in 14d46cc (note it would be recursive "for free" in the call tree with this change) I think we can do it for the marker table first, as we discussed in the past, because it's probably much easier. |
Oh, I forgot to reset it. I probably thought to complicated.
You mean the following? interface Tree<DisplayData: Object> {
getDepth(NodeIndex): number;
getRoots(): NodeIndex[];
getDisplayData(NodeIndex): DisplayData;
getParent(NodeIndex): NodeIndex;
getChildren(NodeIndex): NodeIndex[];
hasChildren(NodeIndex): boolean;
getAllDescendants(NodeIndex): Set<NodeIndex>;
sort(string): Tree<DisplayData>;
} |
I mean that the sorting should happen directly in The TreeView itself would be reponsible for displaying the sorting marker (like you did now I believe), and calling callbacks when the user clicks on them. |
|
I implement all suggestions, but I still have a test error that I don't understand (the code works when I test it manually in the browser). |
|
I fixed the small bug that caused the test to fail. |
|
Ping |
7c72602 to
ad0b56b
Compare
|
I rebased it on the current main branch. |
| className={`treeViewHeaderColumn treeViewFixedColumn ${col.propName} ${sortClass}`} | ||
| data-column={col.propName} | ||
| > | ||
| ></span> |
There was a problem hiding this comment.
This feature will be extremely useful for screen reader users. Thanks for working on it. However, as they are currently, these sort controls won't be usable by screen reader users:
- They should have role="button".
- They should have aria-sort="ascending" if sorted ascending, aria-sort="descending" if sorted descending or no aria-sort attribute if not sorting by this column.
- They should have tabindex="0" to make them focusable. Ideally, the sort controls would all occupy a single tab stop and you'd use the left and right arrows to move between them, but that will take more work. In the interim, I think it's reasonable to have them all reachable with tab. If these extra tab stops are a real problem for non-keyboard screen reader users, although not ideal, this could be dealt with in a follow-up; screen reader users can get to these controls without them being focusable.
|
Hey, I'm also looking for this feature! |
I botched the rebase for #4203, so I created a new PR.
his change adds sorts buttons to the MarkerTable and CallTree, allowing to sort the columns of both views.
An example: Before this change the marker table looked like:
After this change it looks initially like:
Making it clear to the user that sorting is available (and that the table is sorted by the first column). Clicking on the "Start" column changes this to:
Clicking on another column like "Duration" sorts by this column:
It includes tests for the expected behavior.
Deploy preview
┆Issue is synchronized with this Jira Task