fix: Formatting Rule Doesn't use default set by user#1547
fix: Formatting Rule Doesn't use default set by user#1547Zhou-Ziheng merged 6 commits intodeephaven:mainfrom
Conversation
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Some questions for Don, and make sure we don't screw up line endings.
| { | ||
| endOfLine: 'auto', | ||
| }, |
There was a problem hiding this comment.
We shouldn't be changing the endOfLine default, we want to keep it to lf instead of crlf. Might need to configure something on Windows so that line endings are handled correctly in git.
| onChange={this.handleTruncateNumbersWithPoundChange} | ||
| > | ||
| Truncate numbers with # | ||
| Show truncated numbers as ### |
There was a problem hiding this comment.
A contextual help bubble could be useful here... I think the phrase "Mask truncated numbers with ###" may be better as well, @dsmmcken any opinion?
There was a problem hiding this comment.
I am fine with this suggestion. Using the word show is more consistent wit the other labels, like "Show T seperator", and I like the multiple hashtags in the label. It's a bit long now is my only complaint.
There was a problem hiding this comment.
what about Show truncation as ###
There was a problem hiding this comment.
It only applies to numbers though
| defaultFormatString ?? IntegerColumnFormatter.DEFAULT_FORMAT_STRING | ||
| } | ||
| onKeyDown={e => { | ||
| if (e.key === 'Enter' && value === '') { |
There was a problem hiding this comment.
I'm not sure I like this - it's kind of hidden functionality that is difficult for the user to find. Granted, I don't know a better way to start with the default/edit it, other than copying pasting from above.
@dsmmcken Do you have any ideas for creating a new rule based off the default but edited slightly?
There was a problem hiding this comment.
We could just set it to the initial value instead of a placeholder.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1547 +/- ##
==========================================
- Coverage 46.42% 46.38% -0.05%
==========================================
Files 558 564 +6
Lines 35714 35810 +96
Branches 8916 8969 +53
==========================================
+ Hits 16582 16612 +30
- Misses 19082 19146 +64
- Partials 50 52 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
mofojed
left a comment
mofojed
left a comment
There was a problem hiding this comment.
Looks good overall. Just a very minor cleanup.
| if (key === 'columnType') { | ||
| resetKeys = { | ||
| format: '', | ||
| format: this.makeDefaultFormatterItemByType(value as string), |
There was a problem hiding this comment.
I wonder if there's a way using function overloading and/or generics to correct this so you don't need to cast it: https://www.typescriptlang.org/docs/handbook/2/functions.html#function-overloads
Figuring that out isn't really necessary for this though.
| const onTypeChange = (e: ChangeEvent<HTMLSelectElement>): void => { | ||
| this.handleFormatRuleChange(i, 'columnType', e.target.value); | ||
| }; |
There was a problem hiding this comment.
Not sure why you added these here - handleFormatRuleChange doesn't return anything, so you can keep the shorthand. Or update the onNameChange and onNameBlur declarations to match for consistency.
|
@Zhou-Ziheng good work. We need to port the same fix over to Enterprise as well (Settings menu isn't shared currently - that would be another task that would be sweet). |
3 participants
* Pressingenteron empty formatting string input fields should fill the input with the default string* Updated eslint config to handle lines ending in CRLF correctly