-
-
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
[GSoC'24] M1.4 Add features, platform section #20479
Conversation
Assigning @Lawful2002 for the first pass review of this PR. Thanks! |
PTAL @melhikh review the videos posted above. Thanks! |
Both the interaction and the design are correct and nice. |
@AkashPaloju Have you read the wiki page on frontend tests and the section on coverage? There should be a local coverage report file you can look at to see what the issue is. In general when you run into dev stack issues please take a look at the wiki page and see if your doubt is answered in the relevant section, then in your question specify which pages you've looked at. Thanks! |
Yes, I am aware of the test coverage part on the wiki. My doubt is why the test coverage check of the file core/templates/filters/summarize-nonnegative-number.pipe.ts is failing on CI of my PR here. I haven't written this file and the code I removed in this PR haven't used this pipe ( No, such big numbers are there in the old features section that are summarized by this pipe.) Edit : Found the bug the pipe |
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 @AkashPaloju, a couple comments. Also at 0:13
in your first video, clicking on the Explore Lessons
is redirecting to non existent page. I don't think that should happen.
}, | ||
]; | ||
|
||
panelIsCollapsed: boolean[] = [true, true, true, true]; |
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.
Instead of having this here separately, let's include this as part of the featuresData
array items itself. This makes it more maintainable.
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.
Done
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.
I still see this array
text: string; | ||
image?: string; | ||
customPanelClassNames: string[]; | ||
customTitleClassNames: string[]; |
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.
Inline with the previous comment, include a boolean collapsed
here.
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.
Done
Unassigning @Lawful2002 since the review is done. |
Hi @AkashPaloju, it looks like some changes were requested on this pull request by @Lawful2002. PTAL. Thanks! |
Hey Lawful2002 Screencast.from.17-06-24.11.01.18.PM.IST.webm |
@Lawful2002 PTAL at the changes |
Unassigning @AkashPaloju since a re-review was requested. @AkashPaloju, please make sure you have addressed all review comments. Thanks! |
Hi Akash, can you create a classroom, and test it, just to be absolutely sure. |
@Lawful2002 , PTAL at this video. Thanks! Screencast.from.18-06-24.06.23.57.PM.IST.webm |
Hi @AkashPaloju. 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! |
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.
Actually, I don't see my comment resolved. PTAL
}, | ||
]; | ||
|
||
panelIsCollapsed: boolean[] = [true, true, true, true]; |
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.
I still see this array
Unassigning @Lawful2002 since they have already approved the PR. |
|
Hmm can't find it now. Weird |
Hi @AkashPaloju, 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! |
Overview
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
features-desktop-preview.webm
Proof of changes on desktop with slow/throttled network
features-throttle-preview.webm
Proof of changes on mobile phone
features-mobile-preview.webm
Proof of changes in Arabic language
features-arabic-preview.webm
PTAL @Lawful2002
PR Pointers