-
Notifications
You must be signed in to change notification settings - Fork 163
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
Text autocomplete based on column neighbors #1905
Conversation
davidfig
commented
Sep 23, 2024
•
edited
Loading
edited
- rust search of all text neighbors into a string[]
- when one value is found, show it as selected autocomplete in the inline editor
- when more than one value is found, show it as a dropdown box
- limited neighboring value list to 1,000 entries
- make sure it doesn't conflict with validations
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## qa #1905 +/- ##
==========================================
+ Coverage 91.16% 91.17% +0.01%
==========================================
Files 246 246
Lines 54161 54247 +86
==========================================
+ Hits 49374 49459 +85
- Misses 4787 4788 +1 ☔ View full report in Codecov by Sentry. |
const validations = sheets.sheet.getValidation(pos.x, pos.y); | ||
if (validations?.length) { | ||
const validation = validations[0]; | ||
if (validationRuleSimple(validation) === 'logical') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be converted into a switch
statement?
const dropdownKeyboard = (key: 'ArrowDown' | 'ArrowUp' | 'Enter' | 'Escape' | 'Tab') => { | ||
if (!filteredList) return; | ||
|
||
if (key === 'ArrowDown' || key === 'ArrowUp') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good candidate for a switch
statement (might make it more readable/scanable).
const model = this.getModel(); | ||
editor.setModelLanguage(model, language); | ||
this.setBracketConfig(language === 'Formula'); | ||
if (this.editor) { | ||
this.editor.updateOptions({ | ||
quickSuggestions: language === 'Formula' ? true : { other: 'inline' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you Monaco for an api that accepts an object or a boolean value 🤯
@@ -111,6 +111,7 @@ export const HtmlValidationList = (props: Props) => { | |||
// handle keyboard events when list is open | |||
useEffect(() => { | |||
const dropdownKeyboard = (key: 'ArrowDown' | 'ArrowUp' | 'ArrowLeft' | 'ArrowRight' | 'Enter' | 'Escape') => { | |||
debugger; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
@@ -186,6 +188,44 @@ impl Sheet { | |||
}); | |||
results | |||
} | |||
|
|||
/// Returns a Vec<String> of all the neighboring text in the column. Search |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great documentation 💯
} | ||
text.push(t.clone()); | ||
y -= 1; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be able to remove this else block, but that might require the moving of y -= 1
outside of the conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a couple of non-blocking comments.