Rework findColumnIndexForColumnSetting to MLv2 #37854
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 22684b8...5dbd7b5.
|
uladzimirdev
force-pushed
the
mlv2-find-column-settings
branch
from
6917607
to
78f9b7b
Compare
|
uladzimirdev
force-pushed
the
mlv2-find-column-settings
branch
from
2a2d026
to
63022e7
Compare
uladzimirdev
force-pushed
the
mlv2-find-column-settings
branch
from
6f424bf
to
94a9dc2
Compare
* Add a comment why we can't use mlv2 in chart setting table * Use mlv2 findColumnIndexForColumnSetting in ChartSettingTableColumns * Tests completed, so revert throwing
* Use mlv2 findColumnIndexForColumnSetting in object detail * Address review
* Use mlv2 findColumnIndexForColumnSetting in Question * Update question tests
ranquild
approved these changes
Feb 20, 2024
| @@ -1,7 +1,9 @@ | |||
| import cloneDeep from "lodash.clonedeep"; | |||
There was a problem hiding this comment.
structuredClone didn't work?
There was a problem hiding this comment.
Initially jest didn't have it. After adding cljs started raising an error for cloned arrays (fieldRefs). We can actually use structuredClone on FE and map it to deepClone at jest
|
|
||
| // before we fully migrate from Audit v1 | ||
| // it's possible to have internal query here, which throws | ||
| try { |
There was a problem hiding this comment.
You could check for internal queries like this
and it would be easy to find when audit V1 is removed| }: ObjectDetailProps) { | ||
| const [currentObjectIndex, setCurrentObjectIndex] = useState(0); | ||
|
|
||
| // only show modal if this object detail was triggered via an object detail zoom action | ||
| const shouldShowModal = isObjectDetail; | ||
| const getFallbackQuestion = () => |
There was a problem hiding this comment.
Do you know where question is not passed?
There was a problem hiding this comment.
I followed the idea of having a public question without permissions, so dataset query could be empty. I'll try to remove the fallback
|
@uladzimirdev Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #37677
This is an umbrella PR, other changes related to the problem will be merged here
Tasks
Description
Migrate
findColumnIndexForColumnSettingto MLv2 with a fallback to the old method.Throwing an error is added to find all places where
queryis not passed, temporary