Fix part of #10474: Make typescript checks strict for few more files #16214
Conversation
|
Hi @JeeveshGarg please assign the required reviewer(s) for this PR. Thanks! |
|
Hi @JeeveshGarg. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
|
Hi @JeeveshGarg, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
|
Hi @JeeveshGarg, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
|
Hi @JeeveshGarg, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
oppiabot
bot
added
PR: don't merge - HAS MERGE CONFLICTS
and removed
PR: don't merge - HAS MERGE CONFLICTS
labels
|
Hi @JeeveshGarg. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
|
Hi @JeeveshGarg, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
| } | ||
|
|
||
| interface SkillRubrics { | ||
| difficulty: string; | ||
| explanations: string[] | string; | ||
| } | ||
|
|
||
| interface ActiveContributionDetailsDict { | ||
| export interface ActiveContributionDetailsDict { |
There was a problem hiding this comment.
Why does this need to be exported now?
There was a problem hiding this comment.
Done, may be earlier use it in spec.
| keyboard: false, | ||
| }); | ||
| const skillId = this.suggestion.change.skill_id; | ||
| if (skillId) { |
There was a problem hiding this comment.
| if (skillId) { | |
| if (!skillId) { | |
| return; | |
| } |
Allows us to unnest the following code.
| @@ -242,7 +249,11 @@ export class QuestionSuggestionReviewModalComponent | |||
| this.showQuestion = false; | |||
| this.skippedContributionIds.push(this.currentSuggestionId); | |||
|
|
|||
| this.currentSuggestionId = this.remainingContributionIdStack.pop(); | |||
| let CurrentSuggestionId = this.remainingContributionIdStack.pop(); | |||
There was a problem hiding this comment.
Not done? Note that the variable should be uncapitalized as well.
| const CurrentSuggestionId = this.remainingContributionIdStack.pop(); | ||
| if (CurrentSuggestionId === undefined) { |
There was a problem hiding this comment.
| const CurrentSuggestionId = this.remainingContributionIdStack.pop(); | |
| if (CurrentSuggestionId === undefined) { | |
| const currentSuggestionId = this.remainingContributionIdStack.pop(); | |
| if (currentSuggestionId === undefined) { |
There was a problem hiding this comment.
Done, did by mistake.
| @@ -397,19 +407,23 @@ export class TranslationModalComponent { | |||
| getElementAttributeTexts( | |||
| elements: HTMLCollectionOf<Element>, type: string): string[] { | |||
| const textWrapperLength = 6; | |||
| const attributes = Array.from(elements, function(element: Element) { | |||
| let attributes = Array.from(elements, function(element: Element) { | |||
There was a problem hiding this comment.
| let attributes = Array.from(elements, function(element: Element) { | |
| const attributes = Array.from(elements, function(element: Element) { |
| textWrapperLength, attribute.length - textWrapperLength); | ||
| const attribute = element.attributes[ | ||
| type as keyof NamedNodeMap] as Attr; | ||
| const attributeValue = attribute.value; |
There was a problem hiding this comment.
I meant we can combine this statement with the above, but it's fine to leave as is.
|
Unassigning @sagangwee since the review is done. |
|
Hi @JeeveshGarg, it looks like some changes were requested on this pull request by @sagangwee. PTAL. Thanks! |
|
@sagangwee PTAL. Thanks. |
|
Unassigning @JeeveshGarg since a re-review was requested. @JeeveshGarg, please make sure you have addressed all review comments. Thanks! |
|
Unassigning @sagangwee since they have already approved the PR. |
|
@oppia/lace-frontend-reviewers PTAL. |
|
Hi @JeeveshGarg, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
|
Hi @JeeveshGarg, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
| @@ -196,7 +199,10 @@ export class QuestionEditorComponent implements OnInit, OnDestroy { | |||
| } | |||
| this.solutionValidityService.init(['question']); | |||
| const stateData = this.questionStateData; | |||
| stateData.interaction.defaultOutcome.setDestination(null); | |||
| const outcome = stateData.interaction.defaultOutcome; | |||
| if (outcome) { | |||
There was a problem hiding this comment.
Hi @JeeveshGarg -- you should probably be aware that this change has caused a significant regression in the contributor dashboard (#16973) that has completely blocked question reviewers from editing questions. This is because this code assigns a string value to outcome.dest, but question answer groups are not supposed to have destinations since they are not part of an exploration.
Could you please explain why you changed this to this.questionId instead of null, and what sort of testing you did for this component?
|
Note that this PR caused another serious regression in hints, which we are trying to fix with #19261. |
Overview
Hidden list of completed Files
Essential Checklist
Proof that changes are correct
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers