Fix part of #19217: Develop footer component and add conversation skin to new lesson player

[Vir-8]

Overview

1. This PR fixes part of [Feature Request]: Redesign the lesson player #19217.

2. This PR does the following:
   a. Implement the footer component for the new lesson player having -

   • Completely functional previous, continue and submit buttons
   • Save functionality for logged out users

   b. Duplicate the conversation skin and progress nav components for the new lesson player
   c. Create and add the save progress modal for the save functionality

Refer to this doc for more info: Lesson Player Redesign Implementation Plan

Essential Checklist

• The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
• I have followed the instructions for making a code change.
• I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
• The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
• "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Proof that changes are correct

Proof of changes on desktop

User is logged in:

footer-logged-in.mov

User is not logged in:

footer-logged-out.mov

Proof of changes on mobile phone

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.

[oppiabot]

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

[Vir-8]

PTAL @seanlip

Note: I had to unfortunately exclude 2 components from typescript checks because of how current services like playerPositionService work, and it would take a lot of refactoring of current services I think, hope that is OK

[seanlip]

Hi @Vir-8, one top-level comment -- PTAL.

[seanlip]

upto --> up to

[Vir-8]

Done

[seanlip]

Let's talk strategy a bit here ... these are direct copies of the existing components, right? What is your plan for modifying the components -- are you planning to modify the existing ones or are you planning to build the new ones from scratch?

I think what I would actually suggest is the latter (i.e. don't put stuff in here just to modify it later) and the process could be something like the following:

• For any common functionality between the old and new skins, make separate PR(s) that move that common functionality (and its tests) into a service so that you can reuse said functionality with the new skin. Minimize the stuff in the component to just what's needed for the "skin" part.
• Then build a new component that uses the same services.

Basically we want to make sure that anything that undergoes full review ends up being a solid baseline for what follows. Copying the existing files into here kind of defeats that.

[Vir-8]

True, I was planning on modifying the existing ones as we go but I do see your point with undergoing a full review. The purpose of this PR was to focus solely on the footer component, for which the conversation skin is a necessity so I left it mostly untouched.

Maybe it would've been better to get done with the skin first in this case, I was planning on working on it when I work on end-steps. Rather than building it from scratch, I'll modify it and remove what I find is redundant but work on the UI in a later PR, does that work for now?

[seanlip]

Ah I think a better approach is to make a new skin but just have it be really bare-bones (don't worry about its UI, just implement the bare minimum of necessary functionality). But start from empty rather than start from the existing one, otherwise you'll just take the bad patterns of the existing one and replicate them here as well.

I do rather strongly suggest extracting common parts into services though so that there is no copy/paste going on. That way the amount of revisiting needed can be restricted to just the (thin) controller + html layers, and probably only the latter if the controller is done properly at the outset.

[Vir-8]

Alright, I'll do that but I'm a bit confused, by extracting common parts into services, do you mean making something like a new-conversation-skin.service.ts - handling the current services that are reused here, and then import this service to our component?

[seanlip]

Yes, that's right, but I wouldn't call it new-conversation-skin.service.ts because it would be used by both the old and new skins. In fact, its name shouldn't really even have reference to "conversation skin" -- each service should be defined in a self-contained way based on what the service does. That would make more sense than having an amalgam of random services that are together only because they are used in the same conversation skin.

Note that this should be staged -- have a PR (or PRs) extracting the services first for the old skin, and make sure the old skin continues to work with the refactor; no change should be made to the new skin or the user-facing behaviour for the old skin. Then in a separate PR add new functionality to the new skin that (might) make use of the extracted services. But always do the first one separately from the second, since they represent separate concerns -- the first one is a refactor preserving existing behaviour, and the second is implementation of new functionality.

Hope that makes sense; please ask if you have any questions!

[seanlip]

Can you try sketching out the services on a sheet of paper or Figma or something, and showing what the definition of each service is and how they connect to each other? Imagine there was no prior art; how would you organize the breakdown into services so that it's conceptually clear what each service does?

Then let's see what we can do to evolve towards that system. But without a clear picture of where we want to head towards it'll be difficult to evaluate partial steps.

(For clarity, this doesn't mean we need to implement that whole "ideal structure" in this project. But let's at least make sure we know what it is so we aren't making things worse. Then we can punt whatever is possible to punt.)

[Vir-8]

Ok, I can think of 2 main "services" for the conversation flow - initializing the conversation and the continuation functionality. Here's an image of how it currently is for the current conversation skin, not including many smaller functions, hoping it can provide a better picture of the situation.

It feels really entangled looking at the code, with most functions dealing with and handling a lot of services that are all overlapping. Have been trying a lot to figure this out but have been upto no good, can you please help me out with how to proceed under the scope of this project?

[seanlip]

Thanks, great diagram. I think maybe we can organize it like this, see what you think:

• Have a conversation-skin.service.ts file that stores a lessonCards stack (or stores it in a dependent service) and a playerPosition (ditto) and exposes the following:
  • initializePlayer()
  • onSubmitAnswer()
  • navigateForwardByOneCard()
  • navigateBackwardByOneCard()
  • navigateToCheckpoint() (or navigateToMostRecentCheckpoint(), depends on what functionality is available)
• Make the old skin use this service instead of doing what it's doing now (there should be no change in user-facing behaviour, add tests if needed)
• (Separate PR) Make the new skin use this service

This conversation-skin.service.ts can refer to other services like player-position.service.ts etc. as needed. Conceptually, the conversation-skin.service.ts is responsible for keeping track of the list of cards and the player's location, and modifying both of these as needed depending on the user's actions.

Keep other functionality, like hints, in the old skin for now -- we need to move it out later using a similar process as the new skin starts to use it, but for now let's just pull out the core logic that the new skin needs into its own service so that we can implement the new skin without duplicating things.

Does that help? Feel free to suggest a variation of the above if you like, but this is roughly how I'm thinking about the API -- the controller should do little more than just register the user's actions.

[Vir-8]

@seanlip Thanks for the feedback and I'm really sorry for the delay, I've been scratching my head for the past couple of days and still haven't figured it out unfortunately.

The biggest issues I have are that firstly, there are way too many variables being handled currently across the 1700 lines of code, being pretty hard to handle correctly and making it very confusing when trying to refactor the code. It's extremely challenging for me to rewrite this code or extract certain parts of it as it currently is.

And secondly, the fact that it is pretty much necessary to have a lot of seemingly unrelated functions in the service, since our main functions are dependent on them and the tree of small dependency functions can just keep propagating - are we OK with including those?

I want to get it done in the best way possible, but why is it necessary to refactor/rewrite the code of the conversation skin? Initially, I assumed that almost all, if not all current functionalities of the skin will be relevant as the redesign gets closer to being done.

Appreciate your help and really looking forward to your advice, thanks!

[seanlip]

@Vir-8 The main reason we need to do the move here is because otherwise we end up with duplication of code between the old and new skins. If some functionality needs to be modified while this project is in progress, then this increases the danger of skew. This is why we move the common code out into a service so that only the part that needs to be changed gets replaced by the new skin.

If it helps reduce the complexity, one thing you could try doing is to just do some small PRs to "make the existing code better" so that it's easier to extract parts of it. Would that help? I think part of the issue is that you might be trying to do this all at once, and it might be better to do it in smaller stages. One difficulty I have in giving advice here is that I don't know what specific issues you are running into, and I wonder if focusing on making the code better bit by bit will make it easy for you to clarify those.

[oppiabot]

Hi @Vir-8. 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!

[oppiabot]

Hi @Vir-8, 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!