[REVERTED] Fix #12238: Making the question and header sticky in the question editor modal #20127

arjen0203 · 2024-04-03T14:01:06Z

Overview

This PR fixes or fixes part of [Contributor Dashboard] Make question sticky in question editor modal #12238
This PR does the following: It makes the question sticky in the editor and in the review modal of the contributor dashboard
(For bug-fixing PRs only) The original bug occurred because: [Explain what
the cause of the bug was, and which PR introduced it]

Essential Checklist

Please follow the instructions for making a code change.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
~~I have written tests for my code.~~ -> Manually tested since it are css changes, proof provided below.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

Before:
https://github.com/oppia/oppia/assets/14263008/0def067b-f3a8-4053-b099-c5f890660fdc

After:
https://github.com/oppia/oppia/assets/14263008/a9d35a18-7a09-4c99-8883-1c101308815c

Proof of changes in Arabic language

2024-04-03.15-46-14.mov

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

…editor-modal

oppiabot · 2024-04-03T14:01:18Z

Assigning @nikitaevg for the first pass review of this PR. Thanks!

seanlip · 2024-04-03T17:51:14Z

@arjen0203 Quick check -- I just manually linked the issue in the Development section, but why did you tick the first checkbox when that wasn't done? (Wanted to understand if there are any process issues here, thanks!)

nikitaevg

It looks good! But I have a couple concerns, + what Sean said in #12238 (comment)

nikitaevg · 2024-04-03T17:56:07Z

...ges/contributor-dashboard-page/modal-templates/question-suggestion-review-modal.component.ts

@@ -142,6 +142,7 @@ export class QuestionSuggestionReviewModalComponent
  questionId: string | null = null;
  isFirstItem: boolean = true;
  isLastItem: boolean = true;
+  stateContentShouldStayVisibleOnScroll: boolean = true;


If this value is true by default and never changed, why not inlining it in html?

This is value is now specified inline html

nikitaevg · 2024-04-03T17:58:00Z

core/templates/components/question-directives/question-editor/question-editor.component.ts

@@ -60,6 +60,7 @@ export class QuestionEditorComponent implements OnInit, OnDestroy {
  @Input() question!: Question;
  @Input() questionId!: string;
  @Input() questionStateData!: State;
+  @Input() stateContentShouldStayVisibleOnScroll!: boolean;


If this input is required, what about other usages of the question-editor? In my understanding you should pass this argument there too.

Every usage of question-editor now has it's value specified

oppiabot · 2024-04-03T18:01:29Z

Unassigning @nikitaevg since the review is done.

oppiabot · 2024-04-03T18:01:31Z

Hi @arjen0203, it looks like some changes were requested on this pull request by @nikitaevg. PTAL. Thanks!

StephenYu2018

One concern.

core/templates/components/state-editor/state-editor.component.ts

oppiabot · 2024-04-11T17:12:58Z

Hi @arjen0203, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

arjen0203 · 2024-04-12T11:10:00Z

@arjen0203 Quick check -- I just manually linked the issue in the Development section, but why did you tick the first checkbox when that wasn't done? (Wanted to understand if there are any process issues here, thanks!)

That fault lies with me, reading the warning warning of the PR being auto-closed I wronly assumed it would be added automatically. But it is clear to me now that it should have been done manually

arjen0203

Proccesed feedback

arjen0203 · 2024-04-12T12:46:16Z

core/templates/components/question-directives/question-editor/question-editor.component.ts

@@ -60,6 +60,7 @@ export class QuestionEditorComponent implements OnInit, OnDestroy {
  @Input() question!: Question;
  @Input() questionId!: string;
  @Input() questionStateData!: State;
+  @Input() stateContentShouldStayVisibleOnScroll!: boolean;


Every usage of question-editor now has it's value specified

arjen0203 · 2024-04-12T12:49:09Z

...ges/contributor-dashboard-page/modal-templates/question-suggestion-review-modal.component.ts

@@ -142,6 +142,7 @@ export class QuestionSuggestionReviewModalComponent
  questionId: string | null = null;
  isFirstItem: boolean = true;
  isLastItem: boolean = true;
+  stateContentShouldStayVisibleOnScroll: boolean = true;


This is value is now specified inline html

core/templates/components/state-editor/state-editor.component.ts

…editor-modal

StephenYu2018 · 2024-04-12T13:07:04Z

@arjen0203 Never force-push changes to your PR. This will lead to your PR being closed.

arjen0203 · 2024-04-12T13:09:20Z

@arjen0203 Never force-push changes to your PR. This will lead to your PR being closed.

I just read it as I was going over all the steps of the review process again, my apologies, was trying to amend some changes, won't happen again

arjen0203 · 2024-04-12T13:22:52Z

PTAL @StephenYu2018, @nikitaevg

oppiabot · 2024-04-14T09:22:02Z

Unassigning @nikitaevg since they have already approved the PR.

StephenYu2018

LGTM!

oppiabot · 2024-04-17T00:34:18Z

Unassigning @StephenYu2018 since they have already approved the PR.

Ash-2k3 · 2024-04-18T19:33:27Z

Thanks @arjen0203 for fixing this. And Congrats on getting your first PR merged at Oppia !!!

chris7716 · 2024-05-22T10:15:19Z

We need to revert this PR cuz it has introduced a regression #20290

arjen0203 added 3 commits April 2, 2024 17:13

Adds changes made by rassa328

6ff7ace

Improved naming, removed unnecessary code, param is now required

2a4a3e4

Merge remote-tracking branch 'upstream/develop' into sticky-question-…

dfafd10

…editor-modal

arjen0203 requested review from a team as code owners April 3, 2024 14:01

arjen0203 requested review from nikitaevg and StephenYu2018 April 3, 2024 14:01

oppiabot bot assigned nikitaevg Apr 3, 2024

Fixed linting issues

827c903

seanlip linked an issue Apr 3, 2024 that may be closed by this pull request

[Contributor Dashboard] Make question sticky in question editor modal #12238

Open

nikitaevg requested changes Apr 3, 2024

View reviewed changes

oppiabot bot unassigned nikitaevg Apr 3, 2024

oppiabot bot assigned arjen0203 Apr 3, 2024

StephenYu2018 requested changes Apr 4, 2024

View reviewed changes

core/templates/components/state-editor/state-editor.component.ts Show resolved Hide resolved

oppiabot bot added the stale label Apr 11, 2024

oppiabot bot removed the stale label Apr 12, 2024

Param specified everywhere, bool value now inline html

8eb9e1f

arjen0203 force-pushed the sticky-question-editor-modal branch from 5330042 to 8eb9e1f Compare April 12, 2024 12:55

arjen0203 mentioned this pull request Apr 12, 2024

Fix #12238: Making the question sticky in the question editor modal #19288

Closed

6 tasks

arjen0203 commented Apr 12, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into sticky-question-…

628560c

…editor-modal

arjen0203 removed their assignment Apr 12, 2024

oppiabot bot assigned nikitaevg and StephenYu2018 Apr 12, 2024

seanlip mentioned this pull request Apr 12, 2024

[Contributor Dashboard] Make question sticky in question editor modal #12238

Open

nikitaevg approved these changes Apr 14, 2024

View reviewed changes

oppiabot bot unassigned nikitaevg Apr 14, 2024

StephenYu2018 approved these changes Apr 17, 2024

View reviewed changes

oppiabot bot unassigned StephenYu2018 Apr 17, 2024

Ash-2k3 added this pull request to the merge queue Apr 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 18, 2024

Ash-2k3 added this pull request to the merge queue Apr 18, 2024

Merged via the queue into oppia:develop with commit 985e84d Apr 18, 2024
77 checks passed

chris7716 mentioned this pull request May 22, 2024

Revert "Fix #12238: Making the question and header sticky in the question editor modal" #20341

Merged

5 tasks

seanlip changed the title ~~Fix #12238: Making the question and header sticky in the question editor modal~~ [REVERTED] Fix #12238: Making the question and header sticky in the question editor modal May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVERTED] Fix #12238: Making the question and header sticky in the question editor modal #20127

[REVERTED] Fix #12238: Making the question and header sticky in the question editor modal #20127

arjen0203 commented Apr 3, 2024 •

edited

oppiabot bot commented Apr 3, 2024

seanlip commented Apr 3, 2024

nikitaevg left a comment

nikitaevg Apr 3, 2024

arjen0203 Apr 12, 2024

nikitaevg Apr 3, 2024

arjen0203 Apr 12, 2024

oppiabot bot commented Apr 3, 2024

oppiabot bot commented Apr 3, 2024

StephenYu2018 left a comment

oppiabot bot commented Apr 11, 2024

arjen0203 commented Apr 12, 2024

arjen0203 left a comment •

edited

arjen0203 Apr 12, 2024

arjen0203 Apr 12, 2024

StephenYu2018 commented Apr 12, 2024 •

edited

arjen0203 commented Apr 12, 2024 •

edited

arjen0203 commented Apr 12, 2024

oppiabot bot commented Apr 14, 2024

StephenYu2018 left a comment

oppiabot bot commented Apr 17, 2024

Ash-2k3 commented Apr 18, 2024

chris7716 commented May 22, 2024

[REVERTED] Fix #12238: Making the question and header sticky in the question editor modal #20127

[REVERTED] Fix #12238: Making the question and header sticky in the question editor modal #20127

Conversation

arjen0203 commented Apr 3, 2024 • edited

Overview

Essential Checklist

Proof that changes are correct

Proof of changes in Arabic language

PR Pointers

oppiabot bot commented Apr 3, 2024

seanlip commented Apr 3, 2024

nikitaevg left a comment

Choose a reason for hiding this comment

nikitaevg Apr 3, 2024

Choose a reason for hiding this comment

arjen0203 Apr 12, 2024

Choose a reason for hiding this comment

nikitaevg Apr 3, 2024

Choose a reason for hiding this comment

arjen0203 Apr 12, 2024

Choose a reason for hiding this comment

oppiabot bot commented Apr 3, 2024

oppiabot bot commented Apr 3, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 11, 2024

arjen0203 commented Apr 12, 2024

arjen0203 left a comment • edited

Choose a reason for hiding this comment

arjen0203 Apr 12, 2024

Choose a reason for hiding this comment

arjen0203 Apr 12, 2024

Choose a reason for hiding this comment

StephenYu2018 commented Apr 12, 2024 • edited

arjen0203 commented Apr 12, 2024 • edited

arjen0203 commented Apr 12, 2024

oppiabot bot commented Apr 14, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 17, 2024

Ash-2k3 commented Apr 18, 2024

chris7716 commented May 22, 2024

arjen0203 commented Apr 3, 2024 •

edited

arjen0203 left a comment •

edited

StephenYu2018 commented Apr 12, 2024 •

edited

arjen0203 commented Apr 12, 2024 •

edited