fix: Card items overflow out of cards #2658
Conversation
WalkthroughThe changes update the course progress list component by modifying its data source. The Handlebars template now iterates over a new computed property Changes
Sequence Diagram(s)sequenceDiagram
participant T as Template (index.hbs)
participant C as Component (index.js)
participant D as Data (Course Participations)
T ->> C: Request languagesToDisplay for rendering
C ->> D: Retrieve completed participations
D -->> C: Return completed items
C ->> D: Retrieve incomplete participations
D -->> C: Return incomplete items
C ->> C: Sort completed by completion date (desc)
C ->> C: Sort incomplete by last submission date (desc)
C ->> C: Combine items based on MAX_LANGUAGES_TO_DISPLAY
C -->> T: Provide languagesToDisplay array
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/acceptance/view-user-profile-test.js (1)
57-140: Consider extracting test data setup.The test data setup with multiple course participations is quite lengthy and repetitive. Consider extracting this into a helper function or fixture.
Example refactor:
function createCourseParticipation(server, { course, language, user, completedAt, completedStageSlugs, lastSubmissionAt }) { return server.create('course-participation', { course, language, user, ...(completedAt && { completedAt }), ...(completedStageSlugs && { completedStageSlugs }), ...(lastSubmissionAt && { lastSubmissionAt }) }); }Then use it like:
const baseDate = new Date('2021-10-10'); const languages = ['cpp', 'java', 'javascript'].map(slug => this.server.schema.languages.findBy({ slug }) ); languages.forEach(language => createCourseParticipation(this.server, { course: redis, language, user: currentUser, completedStageSlugs: redis.stages.models.sortBy('position').slice(0, 5).mapBy('slug'), lastSubmissionAt: baseDate }) );🧰 Tools
🪛 ESLint
[error] 119-119: Delete
····(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/user-page/course-progress-list-item/index.hbs(1 hunks)app/helpers/limit-array.ts(1 hunks)tests/acceptance/view-user-profile-test.js(3 hunks)
🧰 Additional context used
🪛 ESLint
app/helpers/limit-array.ts
[error] 3-3: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 5-5: Insert ⏎
(prettier/prettier)
tests/acceptance/view-user-profile-test.js
[error] 33-33: 'objectivec' is assigned a value but never used.
(no-unused-vars)
[error] 34-34: 'scala' is assigned a value but never used.
(no-unused-vars)
[error] 35-35: 'shell' is assigned a value but never used.
(no-unused-vars)
[error] 36-36: 'elixir' is assigned a value but never used.
(no-unused-vars)
[error] 119-119: Delete ····
(prettier/prettier)
[error] 187-187: Do not commit pauseTest()
(ember/no-pause-test)
🔇 Additional comments (1)
app/components/user-page/course-progress-list-item/index.hbs (1)
48-48: LGTM! Effectively limits course participation display.The change successfully addresses the card items overflow issue by limiting the display to 3 items.
|
@ryan-gang could you review this please? |
| @@ -45,7 +45,7 @@ | |||
| {{/if}} | |||
|
|
|||
| <div class="flex items-center space-x-1.5"> | |||
| {{#each @courseParticipations as |courseParticipation|}} | |||
| {{#each (limit-array @courseParticipations 3) as |courseParticipation|}} | |||
There was a problem hiding this comment.
Hey @Wassaf001, I don't think just limiting the participations here will work as expected.
We want all these constraints to hold:
- The list of languages should always show the languages in which the participations are
Completed(Max 5, if more than 5 take the 5 most recently active ones). - The list of languages should then add languages with participations which are incomplete, to bring the total count to maximum 5.
- Completed languages should come first.
- Incomplete ones, should be sorted by activity date. (recent ones first).
I think defining a function in app/components/user-page/course-progress-list-item/index.js which accepts courseParticipations and returns this filtered and sorted list to render would be okay.
We do something similar in app/components/user-page/course-progress-list.ts which you can take a look at.
|
Hey @Wassaf001 you can take a look at this PR. |
|
ok will do it at earliest just wait till Sunday. |
|
I am trying to pause the test using : await pauseTest(); but it won't work as expected, before it was working fine. Any guidance please. |
|
it is working using module filter on tests page |
|
done, if any changes required let me know |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/acceptance/view-user-profile-test.js (3)
35-38: 🛠️ Refactor suggestionRemove unused language variables.
The following language variables are declared but never used:
- objectivec
- scala
- shell
- elixir
Remove these unused variables to keep the code clean.
🧰 Tools
🪛 ESLint
[error] 35-35: 'objectivec' is assigned a value but never used.
(no-unused-vars)
[error] 36-36: 'scala' is assigned a value but never used.
(no-unused-vars)
[error] 37-37: 'shell' is assigned a value but never used.
(no-unused-vars)
[error] 38-38: 'elixir' is assigned a value but never used.
(no-unused-vars)
186-186:⚠️ Potential issueRemove debugging pause.
The
pauseTest()call should not be committed as it will pause the test execution.- await this.pauseTest();🧰 Tools
🪛 ESLint
[error] 186-186: Do not commit
pauseTest()(ember/no-pause-test)
6-6: 🛠️ Refactor suggestionImport
pauseTestonly when needed for debugging.The
pauseTestimport is used for debugging but shouldn't be in committed code.-import { currentURL, pauseTest } from '@ember/test-helpers'; +import { currentURL } from '@ember/test-helpers';🧰 Tools
🪛 ESLint
[error] 6-6: 'pauseTest' is defined but never used.
(no-unused-vars)
🧹 Nitpick comments (4)
app/components/user-page/course-progress-list-item/index.js (3)
39-44: Fix whitespace formatting.There's an unnecessary line break after the
@actiondecorator.@action - navigateToCourse() {🧰 Tools
🪛 ESLint
[error] 40-41: Delete
⏎(prettier/prettier)
[error] 44-44: Delete
··(prettier/prettier)
45-62: Well-implementedlanguagesToDisplaymethod to solve overflow issue.The implementation effectively limits card items to prevent overflow by:
- Prioritizing completed courses by most recent completion date
- Adding incomplete courses sorted by recent activity when needed
- Capping the total number of displayed items to 5
However, there are some formatting issues to fix:
get languagesToDisplay() { const completedParticipations = this.completedCourseParticipations.sort( - (a, b) => new Date(b.completedAt).getTime() - new Date(a.completedAt).getTime() + (a, b) => new Date(b.completedAt).getTime() - new Date(a.completedAt).getTime(), ); const incompleteParticipations = this.args.courseParticipations - .filter(p => !p.isCompleted) + .filter((p) => !p.isCompleted) .sort((a, b) => new Date(b.lastSubmissionAt).getTime() - new Date(a.lastSubmissionAt).getTime()); if (completedParticipations.length >= MAX_LANGUAGES_TO_DISPLAY) { return completedParticipations.slice(0, MAX_LANGUAGES_TO_DISPLAY); } - return [ - ...completedParticipations, - ...incompleteParticipations.slice(0, MAX_LANGUAGES_TO_DISPLAY - completedParticipations.length) - ]; + return [...completedParticipations, ...incompleteParticipations.slice(0, MAX_LANGUAGES_TO_DISPLAY - completedParticipations.length)]; }🧰 Tools
🪛 ESLint
[error] 45-62: Member languagesToDisplay should be declared before all method definitions.
(@typescript-eslint/member-ordering)
[error] 47-47: Insert
,(prettier/prettier)
[error] 51-51: Replace
pwith(p)(prettier/prettier)
[error] 58-61: Replace
⏎······...completedParticipations,⏎······...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)⏎····with...completedParticipations,·...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)(prettier/prettier)
45-62: Consider updating member ordering.According to the static analysis tool, the
languagesToDisplaygetter should be declared before method definitions.Move this getter before the
navigateToCoursemethod to follow the project's style guidelines.🧰 Tools
🪛 ESLint
[error] 45-62: Member languagesToDisplay should be declared before all method definitions.
(@typescript-eslint/member-ordering)
[error] 47-47: Insert
,(prettier/prettier)
[error] 51-51: Replace
pwith(p)(prettier/prettier)
[error] 58-61: Replace
⏎······...completedParticipations,⏎······...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)⏎····with...completedParticipations,·...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)(prettier/prettier)
tests/acceptance/view-user-profile-test.js (1)
56-140: Consider simplifying test setup by reducing redundant course participations.While adding multiple course participations helps test the fix for card overflow, there's a lot of repetitive code. Consider using a loop or helper function to create these test entries.
// Example of a more concise approach const languages = ['cpp', 'java', 'javascript', 'haskell', 'c', 'rust', 'ruby']; languages.forEach(langSlug => { const language = this.server.schema.languages.findBy({ slug: langSlug }); this.server.create('course-participation', { course: redis, language: language, user: currentUser, completedStageSlugs: redis.stages.models.sortBy('position').slice(0, 5).mapBy('slug'), lastSubmissionAt: new Date('2021-10-10'), }); }); // Then create the completed ones separately const completedLanguages = [ { slug: 'php', date: '2020-01-01' }, { slug: 'typescript', date: '2021-10-10' }, { slug: 'kotlin', date: '2021-10-11' } ]; completedLanguages.forEach(item => { const language = this.server.schema.languages.findBy({ slug: item.slug }); this.server.create('course-participation', { course: redis, language: language, user: currentUser, completedAt: new Date(item.date), }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/user-page/course-progress-list-item/index.hbs(1 hunks)app/components/user-page/course-progress-list-item/index.js(2 hunks)tests/acceptance/view-user-profile-test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/user-page/course-progress-list-item/index.hbs
🧰 Additional context used
🪛 ESLint
app/components/user-page/course-progress-list-item/index.js
[error] 40-41: Delete ⏎
(prettier/prettier)
[error] 44-44: Delete ··
(prettier/prettier)
[error] 45-62: Member languagesToDisplay should be declared before all method definitions.
(@typescript-eslint/member-ordering)
[error] 47-47: Insert ,
(prettier/prettier)
[error] 51-51: Replace p with (p)
(prettier/prettier)
[error] 58-61: Replace ⏎······...completedParticipations,⏎······...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)⏎···· with ...completedParticipations,·...incompleteParticipations.slice(0,·MAX_LANGUAGES_TO_DISPLAY·-·completedParticipations.length)
(prettier/prettier)
[error] 62-63: Delete ⏎
(prettier/prettier)
tests/acceptance/view-user-profile-test.js
[error] 6-6: 'pauseTest' is defined but never used.
(no-unused-vars)
[error] 35-35: 'objectivec' is assigned a value but never used.
(no-unused-vars)
[error] 36-36: 'scala' is assigned a value but never used.
(no-unused-vars)
[error] 37-37: 'shell' is assigned a value but never used.
(no-unused-vars)
[error] 38-38: 'elixir' is assigned a value but never used.
(no-unused-vars)
[error] 186-186: Do not commit pauseTest()
(ember/no-pause-test)
🔇 Additional comments (1)
app/components/user-page/course-progress-list-item/index.js (1)
6-6: Good implementation ofMAX_LANGUAGES_TO_DISPLAYconstant.This is a good approach to limit the number of language cards shown, which helps prevent card items from overflowing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/acceptance/view-user-profile-test.js (1)
24-24: Remove unnecessary whitespace.This empty line with whitespace should be removed.
-🧰 Tools
🪛 ESLint
[error] 24-24: Delete
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/acceptance/view-user-profile-test.js(2 hunks)
🧰 Additional context used
🪛 ESLint
tests/acceptance/view-user-profile-test.js
[error] 6-6: 'pauseTest' is defined but never used.
(no-unused-vars)
[error] 24-24: Delete ··
(prettier/prettier)
|
Linter is failing, can you also revert all changes done to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/acceptance/view-user-profile-test.js (1)
253-253: Code Formatting: Insert Newline at End of FileStatic analysis (prettier) suggests that a newline should be added at the end of the file to satisfy formatting standards. This will help prevent editor or CI warnings regarding end-of-file newlines.
A possible diff to resolve this:
- }); -} + }); +} +⏎🧰 Tools
🪛 ESLint
[error] 253-253: Insert
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/acceptance/view-user-profile-test.js(1 hunks)
🧰 Additional context used
🪛 ESLint
tests/acceptance/view-user-profile-test.js
[error] 253-253: Insert ⏎
(prettier/prettier)
|
Is there anything else, I need to do? Why is the linter taking so long to show results. |
|
@Wassaf001 CI jobs won't run without approval from members with write access. |
ok understood |
|
Fixed It. Please check. |
|
Why are the tests failing? Please guide me on what to do next. |
|
This is an issue with Codecov on forked repos, will have to update our CI tests to accommodate for this. |
|
ok thank you |
Checklist:
[percy]in the message to trigger)Summary by CodeRabbit