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
Fix #19816 Change where headerText and subheaderText value are set and fix their display on frontend #20219
base: develop
Are you sure you want to change the base?
Conversation
…et and fix their display on frontend
Assigning @Lawful2002 for the first pass review of this PR. Thanks! |
@seanlip PTAL |
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.
Thanks @Sepremo! It looks good to me in general, just had one comment.
Also I have a question -- there are a bunch of issues filed for the ExpressionChangedAfterItHasBeenCheckedError -- see https://github.com/oppia/oppia/issues?q=is%3Aissue+is%3Aopen+expressionchangedafterithasbeencheckederror . Would you mind taking a look and seeing if your PR fixes any of these (or augmenting it to fix some of those if it's easy to do)? If it's not easy then perhaps leaving a comment on the issues you skip explaining what contributors should do might be helpful (but I think it would be quite nice if we could get rid of all of them in one go :P ).
Thanks!
@@ -80,7 +90,7 @@ export class StoryEditorNavigationService { | |||
} | |||
|
|||
navigateToChapterEditor(): void { | |||
this.navigateToChapterEditorWithId(this.chapterId, null); | |||
this.navigateToChapterEditorWithId(this.chapterId, null, ''); |
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.
Bit concerned about this -- why is the empty string being used as the title?
(Also is the function name correct? It seems a bit odd to me that the index is null ... perhaps some jsdoc might be useful for navigateToChapterEditorWithId() explaining what the null case means, and we might need to update this function's name if it is not actually as general as it sounds.)
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.
The comment on line 35 states:
// 'chapterIndex' is null when we are navigating to a chapter with its ID.
It appears that this function was solely utilized in the story-editor-page.component.ts
file to open the Chapter Editor Tab when the page was refreshed or directly accessed via the page link, without needing to assign the chapterIndex
value. This approach aimed to avoid waiting for the story
object to load.
While refreshing the Chapter Editor page, I observed that it results in the chapter title not being updated in the NavigationBar, remaining as the Story Title. Since the chapter title can only be obtained from a node within the story
object, I believe the only viable solution is to replace the usage of the navigateToChapterEditor()
function with an asynchronous wait for the story to be loaded, followed by the utilization of the function navigateToChapterEditorWithId()
with all the necessary values. Is this an appropriate solution, or would displaying the incorrect title while the story object loads and subsequently updating it to the correct one be preferred?
In my locally run server, implementing an asynchronous wait for the story seems to marginally slow down the page refresh by 0.5 seconds.
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.
Hi @Sepremo, thanks for digging! I think it is OK to impose a slight wait there, especially since the page isn't learner-accessible. Want to go ahead and do that? I think it'd be simpler to maintain things if we always used "correct" data throughout.
Thanks for digging into this!
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.
Hey @seanlip,
I reviewed other issues with similar errors and found that their fixes likely won't be as straightforward. Some of those issues involve specific image assets, and another pertains to modifying the isEditable
value, although its exact location is unclear. These issues appear to require different solutions and I don't think I would be able to fix them that easily. I'll leave informative comments where I can.
Additionally, issue #18573 seems to be identical to this one and should be closed once this issue is resolved.
I'll continue to try to fix the tests that are having errors.
Let me know if you need any further adjustments!
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.
Thanks for pointing out the duplicate, I closed that issue!
OK re the other issues, no worries about that. Let's try to get this PR in!
Also I noticed your reply here is for the general comment but not the specific one in this thread. Do you want to respond to the latter as well?
Hi @Sepremo, just wanted to check, if there's any update on this ? |
Hi @Ash-2k3, sorry, I've been a bit busy but I am going to see if it's easy to fix the other similar issues. In the meantime I'll add the changes to fix |
…ll navigateToChapterEditorWithId call
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.
Just one comment, also please look at the failing tests, thanks!
...plates/components/common-layout-directives/navigation-bars/top-navigation-bar.component.html
Show resolved
Hide resolved
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.
Thanks @Sepremo, the changes look good to me! Please fix the failing tests though.
Unassigning @Lawful2002 since they have already approved the PR. |
…ctuation to comment
Overview
headerText
andsubheaderText
values from the Chapter Editor component (story-node-editor.component.ts
) to the component responsible for the navigation of the Story Editor (instory-editor-navigation.service.ts
), like it is done for the updates of SubTopic values in the component for routing of the Topic Editor (intopic-editor-routing.service.ts
)headerText
(along withsubheaderText
) property undergoes updates after Angular's change detection cycle has concluded. This behavior stems from the component managing the new Chapter Editor Tab. The issue arises because it alters a bound value (as bound in thetop-navigation-bar.component.html
) within a new Tab (Chapter Editor), albeit within the same page (Story Editor).This error came from the code migration in Migration of story-editor-page #17968.
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
19816.Fixed.mp4