oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.63k stars 3.78k forks source link

Fix part of #18384: Reusable card component for learner dashboard redesign #20185

Closed amyyeung17 closed 1 day ago

amyyeung17 commented 3 weeks ago

Overview

This PR addresses part of the 2nd breakpoint of https://github.com/oppia/oppia/issues/18384, adding a new reusable card component.

Includes 3 new files:

This reusable card component essentially consolidates these files in summary-tile beginning with:

Additional notes:

Essential Checklist

Please follow the instructions for making a code change.

Proof that changes are correct

https://github.com/oppia/oppia/assets/28910543/026dd92b-8e54-40e1-9a46-8445a00a73f2

oppiabot[bot] commented 3 weeks ago

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

oppiabot[bot] commented 3 weeks ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 3 weeks ago

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

amyyeung17 commented 3 weeks ago

Please try to make concise PRs, I'm not sure I follow why you add changes in the pages/learner-dashboard-page directory. If they are not related to the main change in the PR please don't add them.

Ah sorry, I'll keep the first part in mind. Regarding the changes in pages/learner-dashboard-page, explorations and collections were only retrieved when a user manually clicks the Community Lessons button in the sidebar. I had to move some of the code to use the data in the home tab.

oppiabot[bot] commented 2 weeks ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 2 weeks ago

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

oppiabot[bot] commented 2 weeks ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 2 weeks ago

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

oppiabot[bot] commented 2 weeks ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 2 weeks ago

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

oppiabot[bot] commented 1 week ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 1 week ago

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

amyyeung17 commented 1 week ago

Should this work with rtl languages? If so, please confirm that the current solution works with the Arabic language, or fix it in the next PR.

Apart from that, this might be the last portion of comments for this PR

@nikitaevg It works for the most part. There's just some minor adjustments that I'll handle in a different PR. Might also need to file an issue for the console error that appeared when switching over to Arabic.

https://github.com/oppia/oppia/assets/28910543/3ca6b330-a7d6-43a9-8cc8-ed9a7c685996

oppiabot[bot] commented 1 week ago

Unassigning @nikitaevg since the review is done.

oppiabot[bot] commented 1 week ago

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

oppiabot[bot] commented 4 days ago

Unassigning @nikitaevg since they have already approved the PR.

oppiabot[bot] commented 4 days ago

Unassigning @DubeySandeep since they have already approved the PR.

oppiabot[bot] commented 4 days ago

Hi @amyyeung17, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!