Move Expression Names to Column Settings Header #2437
Conversation
ada-x64
force-pushed
the
features/column-name-in-header
branch
2 times, most recently
from
dbb9e85 to
33cf92b
Compare
ada-x64
force-pushed
the
features/column-name-in-header
branch
from
33cf92b to
cfa46f8
Compare
There was a problem hiding this comment.
Thanks for the PR!
This has some bugs and feature regressions that need to be fixed up:
- We've lost the default naming feature - this was also lost in Expressions API Updates #2399 and re-added in Fix expression alias UX/API #2431. Please add a test for this case as well - we really should have caught this and I'm quite surprised the regression made it this far as it involved deleting alot of code, twice. Was this a rebase gone wrong?
- The title clear button has disappeared - regression of the
<input type="search">. - In these invalid cases - the save button still works and lets you save columns with broken names.
Before - title reflects the default when empty and has a clear button:
Screen.Recording.2023-11-29.at.12.46.03.AM.mov
Now - title shows an error state that was added, but this is incompatible with the default naming functionality which was removed (?), and the clear button is gone:
Screen.Recording.2023-11-29.at.12.46.58.AM.mov
split sidebar into components
error on empty; don't autosave on blur tests
Remove column selector close button when column settings is open. Ignore failing promise in code editor. update snapshots
Co-authored-by: Davis Silverman <[email protected]>
empty header styling empty expression names add test
ada-x64
force-pushed
the
features/column-name-in-header
branch
from
cfa46f8 to
02ec5b1
Compare
|
I've updated this PR to address the feedback.
I'm inclined to say that an unsaved title should be updated when the expression is saved because we are currently closing the column settings sidebar on change. However, with #2459 we'll be keeping the sidebar open. In that case, I'm inclined to say that saving the title and the expression content should be separate operations. |
|
This is really well done! However, it honestly it took me a few minutes to understand what was going on :) My confusion came from muscle-memory, I tried editing an expression column, then changing the title, but not pressing "enter", instead using the mouse to move focus to the expression editor. In this case, the "title" control shows the dashed-border "edited" state, but the expression editor save button is disabled. From this state it wasn't obvious to me that I needed to re-focus the "title" input and press "enter" to submit the changed title! |
I agree, the UX on this is pretty confusing. I originally wanted to do an auto-save feature, where the column title would update on blur. That lead to an awful experience where the sidebar would close all the time. That was the impetus for #2459. I'd be happy to talk more about the UX with this feature; fwiw auto-save felt very nice, and it was cool to see the name automatically update in three places (in the datagrid, the title itself, and the settings panel). Alternative approaches could be things like a clearer "unsaved" state, perhaps an icon with a hover that tells the user to press enter (though this is "polite instructions", i.e. bad UX). We could also hook the title into the expression editor save button, though I think this gets confusing when, for example, you want to change the title of an expression column but you have the style tab pulled up. (We could make it uneditable unless you have the attributes tab open, but that's another hidden functionality...) |
|
Closing in favor of #2459 |
Based on #2436 and #2421This PR moves expression names to the column settings header.
To save the expression, press enter or tab.
Currently when you save the expression name the panel closes. I was originally planning on having the column auto-save, but keeping the panel open requires some significant changes.