-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[Feature Request]: Redesign the lesson player #19217
Comments
Also /cc @juliafalarini, the designer for this project, who can answer questions if needed. |
I would like to work on this!Please assign |
@seanlip sir is this issue still opwn to work on ? |
@yash1378, please do not comment on a whole bunch of issues with the same question. This creates a lot of unnecessary email noise. We have guidelines at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue for new contributors. Please read that page carefully and follow those instructions. It provides info on what issues to pick and the steps you need to take to claim them. For this issue, I recommend that you do not take it as a first issue. It is large and not straightforward, and I recommend that you consider picking something smaller instead to start with. Thanks. |
@seanlip so sorry sir i will take of that and in future will not make unnecessary comments |
@seanlip Hey, while this is pretty huge, I'd love to work on it if possible! I checked out the Figma designs thoroughly, we'd have to refactor a lot in the exploration-player-page directory, mainly -
Order of approach -
Let me know if I'm missing something! Only things I'm not 100% sure about are the tooltips and the next steps. Would really appreciate any advice or suggestions. Thanks! |
Hi @Vir-8, thanks for the notes! Normally I am a bit reluctant to assign a larger project like this to newer folks, but I think your recent PRs have been great so I'd be happy with you taking it up :) Some notes:
I think your overall order of tackling things seems good. I suggest making separate small PRs rather than a huge one, it'll be easier to review. I'm not sure what your concern/question is about the tooltips and next steps. Could you clarify the specific question you have, please? In any case, I'll assign you to this issue -- I suggest that you break it down into milestones and list them here with a planned due date for each. Then we can use that as a basis for planning. Thanks, and feel free to ask if you have further questions! |
Hey @seanlip, thank you so much for the response, means a lot! It's amazing to have this opportunity, I really appreciate your advice. I'll be coming up with a timeline soon, I don't have any concerns regarding next steps now but regarding tooltips - I'm not quite sure how to integrate them into the player, would you be aware of any PR that could serve as a reference? Thanks again! |
@Vir-8 Not sure, will need some investigation. Try to do it in a generic way though -- e.g. make a common component and reuse it. |
@seanlip Here are some tentative target dates by which the corresponding step should be completed, certainly don't want to underestimate how long it would take, please take a look 20 Jan - Make the sidebar component and move components according to the new layout (Will be travelling from 21st-28th) 2 Feb - Have the sidebar component ready, and make the layout responsive Hope this seems OK, don't want to rush it but not sure if this is a lot of time, kindly let me know if you have any feedback or suggestions |
Hi @Vir-8 -- this looks roughly fine. Thanks! Look forward to seeing your first PR. |
@seanlip Just confirming, I'd have to make a PR introducing the feature flag and whatever changes there may be first, and after it gets merged, I will be making following PRs from the same branch? |
The first part is correct, but you should generally make subsequent changes by checking out a new branch from the develop branch after the first PR is merged, so that you get others' changes in as well. |
Add feature flag for lesson player redesign
Hey @seanlip, I have the second PR ready with the routing changes and a rough layout, but I am repeatedly facing this unusual error trying to push the changes, it says linter checks failed but doesn't say what the linting issues are... Already fixed a bunch of linting issues and then this started happening. Any suggestions? |
@Vir-8 Can't help you on this one, sorry, I'm a bit overloaded atm. My advice is to look at the stacktrace and the line in question -- start from the error message. If you need to log any info to get more clarity then go ahead and do that. But it looks like these problems are valid and need to be fixed. |
Hi @Vir-8 , |
@prafulbbandre Mostly by 30th, I am unavailable this week |
Hey @seanlip, shall I wait for previous PRs to be accepted before making new ones or how should I go about it? Since new PRs would include changes from previous PRs that haven't been merged yet |
@Vir-8 OK to make new PRs that are checked-out from the branch you're working on. Just merge the "closer to develop" branch into that branch (and develop to the branch once the older PR is merged) every so often so it doesn't fall out of date. That said, dealing with chains of PRs is more complex. So, feel free to also ping reviewers and ask them to expedite review if possible; make sure to explain that you have other PRs that are blocked on this one. |
…/lesson route (#19635) * Make routing changes for /lesson route * Make service changes to make explorations work under the '/lesson' route * Replicate the exploration player under the '/lesson' route * Make lesson player header component * Make sidebar component and change layout accordingly * Make lesson player footer component * Make audio bar component * Make sidebar expandable and add shadow * Clean code and fix comments * Write tests for sidebar component * Gate the '/lesson' route behind the feature flag and make a README * Fix linting issues * Fix linting issues * Fix linting issues * Fix header test imports * Fix new lesson page guard test * Fix sidebar toggle testing * Fix sidebar and auth guard unit testing * Remove duplicate declarations * Handle imports to access old player components in new player * Fix header text colour * Add player redesign doc to the readme * Update FeatureStatusChecker import after merge * Fix typos
…/lesson route (#19635) * Make routing changes for /lesson route * Make service changes to make explorations work under the '/lesson' route * Replicate the exploration player under the '/lesson' route * Make lesson player header component * Make sidebar component and change layout accordingly * Make lesson player footer component * Make audio bar component * Make sidebar expandable and add shadow * Clean code and fix comments * Write tests for sidebar component * Gate the '/lesson' route behind the feature flag and make a README * Fix linting issues * Fix linting issues * Fix linting issues * Fix header test imports * Fix new lesson page guard test * Fix sidebar toggle testing * Fix sidebar and auth guard unit testing * Remove duplicate declarations * Handle imports to access old player components in new player * Fix header text colour * Add player redesign doc to the readme * Update FeatureStatusChecker import after merge * Fix typos
Hey @seanlip, were the TS tests changed recently or something? I've been trying to make the next PR but I fail a large amount of typescript checks for completely unrelated code that I haven't touched at all, like ![]() |
Hi @Vir-8 -- sorry, I don't know offhand, but I do know something that might be related. IIRC there is a bug where, if you have made a particular TS typing error, all the existing errors in the codebase get printed out. So fixing the error you introduced might fix this. I think this is because the "exclude files" logic in the TS script is behaving incorrectly but this hasn't been fixed yet. See #17236 |
…e at /lesson route (oppia#19635) * Make routing changes for /lesson route * Make service changes to make explorations work under the '/lesson' route * Replicate the exploration player under the '/lesson' route * Make lesson player header component * Make sidebar component and change layout accordingly * Make lesson player footer component * Make audio bar component * Make sidebar expandable and add shadow * Clean code and fix comments * Write tests for sidebar component * Gate the '/lesson' route behind the feature flag and make a README * Fix linting issues * Fix linting issues * Fix linting issues * Fix header test imports * Fix new lesson page guard test * Fix sidebar toggle testing * Fix sidebar and auth guard unit testing * Remove duplicate declarations * Handle imports to access old player components in new player * Fix header text colour * Add player redesign doc to the readme * Update FeatureStatusChecker import after merge * Fix typos
…d improve responsiveness (#19808) * Make routing changes for /lesson route * Make service changes to make explorations work under the '/lesson' route * Replicate the exploration player under the '/lesson' route * Make lesson player header component * Make sidebar component and change layout accordingly * Make lesson player footer component * Make audio bar component * Make sidebar expandable and add shadow * Clean code and fix comments * Write tests for sidebar component * Gate the '/lesson' route behind the feature flag and make a README * Fix linting issues * Fix linting issues * Fix linting issues * Fix header test imports * Fix new lesson page guard test * Fix sidebar toggle testing * Fix sidebar and auth guard unit testing * Remove duplicate declarations * Handle imports to access old player components in new player * Fix header text colour * Add sidebar buttons with SVGs * Add mobile menu button to layout * Add player menu for mobile layouts * Fix responsiveness and add mobile menu options * Fix layout responsiveness * Stylize sidebar expansion toggle * Add lesson description to sidebar * Display lesson ratings in sidebar * Fix fetching ratings from backend * Improve mobile menu frontend * Fix typos * Write tests for mobile menu service * Fix linter errors and reuse current rating backend api service * Modify rating backend api service to access overall_ratings from the response * Fix linting errors * Fix typescript issues * Make a new service for fetching ratings to not interfere with existing services * Update tests to account for handling redirects manually * Fix sidebar to be in the context of an exploration * Fix player sidebar unit tests * Improve handling rating star SVGs in sidebar * Fix layout for smaller landscape screens * Fix sidebar unit tests * Fix sidebar unit tests * Fix sidebar unit tests * Fix sidebar and header unit test coverage issues * Switch from custom SVG icons to fontawesome icons * Remove ratings from sidebar component * Fix linting issues * Improve phrasing for clarity and internationalize strings
…lity to service (#20135) * Migrate core card addition functionality and tests to common service * Remove unused imports * Improve naming and comments for clarity * Move isPlayerAnimating function to avoid calling service from HTML * Add comments for clarity regarding display conditions * Rename conversation-skin service and move function to avoid service calls in HTML * Add unit tests * Fix comment * Address comments * Address comments * Revert "Address comments" This reverts commit f2132755c9d38e9e5b067c61d9640357ee7a693c. * Revert "Address comments" This reverts commit 39b59cca054e3ffe83289746993ae93f60fb49fe. * Fix test * Remove redundant imports * Remove redundant window mock * Address comments * Address reviewer comments * Fix linter issues
Is your feature request related to a problem? Please describe.
The aim of this project is to redesign the lesson player based on feedback we have received from users. We want to make it intuitive to navigate, easy to add features to in the future, and more engaging for younger audiences.
All features of the lesson player must work well on mobile, desktop and tablet device sizes, and all features should work in all languages (including both RTL and LTR languages).
See https://docs.google.com/document/u/1/d/1922aE9_TEFTbHyA3jrXy9cTxo2JgvjfEvKHlxqxG-Rw/edit for a mini-PRD.
Describe the solution (or solutions) you'd like
The lesson player should be redesigned according to these mocks (see the columns on the right half of the Figma file): https://www.figma.com/file/YWe7SqfUVjxlJLKTUn0UZa/Project-2?type=design&node-id=7076-365949&mode=design&t=v3mhfxAkI0l9V53z-0 . It should work fully on mobile and desktop.
From a technical standpoint, this is a large project. I suggest that we gate this behind a feature flag and develop the feature behind the /lesson URL (which can, for now, redirect to the same backend handlers as /explore). The order of development could probably follow the order of the mocks:
Note that the new UI must work fully on desktop and mobile before it can be launched.
Describe alternatives you've considered and rejected
N/A
Additional context
This is filed based on the design issue here: oppia/design-team#58
The text was updated successfully, but these errors were encountered: