-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Web console: Remove first / last suggestions #10794
Conversation
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.
LGTM. I left a nit comment on the code style.
// { | ||
// group: 'first', | ||
// suggestions: ['longFirst', 'doubleFirst', 'floatFirst'], | ||
// }, | ||
// { | ||
// group: 'last', | ||
// suggestions: ['longLast', 'doubleLast', 'floatLast'], | ||
// }, |
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.
We don't usually allow commented out code in java code. Do you think it's better to be consistent in the web-console as well?
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.
fixed.
I don't think this fixes #10702 IMO it should still be an open issue, this fix makes it less likely that someone will trip on this from the UI |
@FrankChen021 that's great news! Looking forward to the PR! |
* Remove first / last suggestions * remove commened out code
* Remove first / last suggestions * remove commened out code Co-authored-by: Vadim Ogievetsky <[email protected]>
This reverts commit 2a1e47a.
Comment out some optimistic suggestion until such a time when they will be possible at ingestion time.
Related to #10702