-
Notifications
You must be signed in to change notification settings - Fork 66
[IMP] dv: autofocus input #7252
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,8 +41,8 @@ export class CriterionInput extends Component<Props, SpreadsheetChildEnv> { | |
| setup() { | ||
| useEffect( | ||
| () => { | ||
| if (this.props.focused) { | ||
| this.inputRef.el!.focus(); | ||
| if (this.props.focused && this.inputRef.el) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... can the input ref really be undefiend at useEffect ? Or is it just to avoid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, if the input isn't displayed because it's a composer instead |
||
| this.inputRef.el.focus(); | ||
| } | ||
| }, | ||
| () => [this.props.focused, this.inputRef.el] | ||
|
|
@@ -94,6 +94,7 @@ export class CriterionInput extends Component<Props, SpreadsheetChildEnv> { | |
| defaultRangeSheetId: this.env.model.getters.getActiveSheetId(), | ||
| invalid: this.state.shouldDisplayError && !!this.errorMessage, | ||
| defaultStatic: true, | ||
| autofocus: this.props.focused, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ interface Props { | |
| interface State { | ||
| rule: DataValidationRuleData; | ||
| errors: CancelledReason[]; | ||
| isTypeUpdated: boolean; | ||
| } | ||
|
|
||
| export class DataValidationEditor extends Component<Props, SpreadsheetChildEnv> { | ||
|
|
@@ -44,7 +45,11 @@ export class DataValidationEditor extends Component<Props, SpreadsheetChildEnv> | |
| onCloseSidePanel: { type: Function, optional: true }, | ||
| }; | ||
|
|
||
| state = useState<State>({ rule: this.defaultDataValidationRule, errors: [] }); | ||
| state = useState<State>({ | ||
| rule: this.defaultDataValidationRule, | ||
| errors: [], | ||
| isTypeUpdated: false, | ||
| }); | ||
| private editingSheetId!: UID; | ||
|
|
||
| setup() { | ||
|
|
@@ -62,6 +67,7 @@ export class DataValidationEditor extends Component<Props, SpreadsheetChildEnv> | |
|
|
||
| onCriterionTypeChanged(type: DataValidationCriterionType) { | ||
| this.state.rule.criterion.type = type; | ||
| this.state.isTypeUpdated = true; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should probably have the same behaviour for CFs and in the data filter menu, no ? To confirm with frgi, but I don't see why implement it in DVs but not CFs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's done 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm blind |
||
| } | ||
|
|
||
| onRangesChanged(ranges: string[]) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -268,16 +268,26 @@ CancelledReasons: ${this.utils.printReceived(dispatchResult.reasons)} | |
| return { pass: false, message: () => message }; | ||
| } | ||
| const pass = element.classList.contains(expectedClass); | ||
| const message = () => | ||
| pass | ||
| ? "" | ||
| : `expect(target).toHaveClass(expected);\n\n${this.utils.printDiffOrStringify( | ||
| expectedClass, | ||
| element.className, | ||
| "Expected class", | ||
| "Received class", | ||
| false | ||
| )}`; | ||
| const message = () => { | ||
| if (this.isNot && pass) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we do something generic ? All of the other custom matchers are also wrong ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's do this in another task |
||
| return `expect(target).not.toHaveClass(expected);\n\n${this.utils.printDiffOrStringify( | ||
| expectedClass, | ||
| element.className, | ||
| "Unexpected class", | ||
| "Received class", | ||
| false | ||
| )}`; | ||
| } else if (!pass) { | ||
| return `expect(target).toHaveClass(expected);\n\n${this.utils.printDiffOrStringify( | ||
| expectedClass, | ||
| element.className, | ||
| "Expected class", | ||
| "Received class", | ||
| false | ||
| )}`; | ||
| } | ||
| return ""; | ||
| }; | ||
| return { pass, message }; | ||
| }, | ||
| toHaveAttribute(target: DOMTarget, attribute: string, expectedValue: string) { | ||
|
|
||
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.
Do you think it makes sense to add it for other CF as well ?

For instance ColorScale has a set iof inputs -
and so does Icon set (but that one is a bit far fetched
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.
from a functional POV, probably yes it makes sense. From a technical POV, is it worth it ? I don't know...
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.
let's keep it as it is for now then