Skip to content

Update Assessment Editor layout and card structure#5924

Open
Abhishek-Punhani wants to merge 3 commits into
learningequality:unstablefrom
Abhishek-Punhani:Issue5912
Open

Update Assessment Editor layout and card structure#5924
Abhishek-Punhani wants to merge 3 commits into
learningequality:unstablefrom
Abhishek-Punhani:Issue5912

Conversation

@Abhishek-Punhani
Copy link
Copy Markdown
Member

Summary

Updated the Assessment Editor Layout according to the new design

This PR:

  • Replaces the legacy VCard layout with KPageContainer for individual question cards and the "Show answers" checkbox.
  • Introduces a scannable header for active questions displaying "Question X of Y" alongside move controls.
  • Updates the AssessmentItemPreview to mirror the new card structure, showing "Question X of Y — [Question type]" in the header.
  • Migrates the response type selector to KSelect and adds a "Type" label above it.
  • Reduces the default minimum height of the TipTapEditor and adds a visual divider line before the Hints section.
  • Ensures a minimum of 16px vertical separation between all question cards.

References

Fixes #5912

Reviewer guidance

To test these changes:

  1. Open a channel in the Studio channel editor.
  2. Select or create an Exercise node.
  3. In the edit panel, navigate to the Questions tab.
  4. Add a few questions and verify both the preview and editor states.

AI usage

Codex assisted me in implementing the changes and final nitpicking. I have carefully reviewed all the changes.

Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
@learning-equality-bot
Copy link
Copy Markdown

👋 Hi @Abhishek-Punhani, thanks for contributing!

For the review process to begin, please verify that the following is satisfied:

  • Contribution is aligned with our contributing guidelines

  • Pull request description has correctly filled AI usage section & follows our AI guidance:

    AI guidance

    State explicitly whether you didn't use or used AI & how.

    If you used it, ensure that the PR is aligned with Using AI as well as our DEEP framework. DEEP asks you:

    • Disclose — Be open about when you've used AI for support.
    • Engage critically — Question what is generated. Review code for correctness and unnecessary complexity.
    • Edit — Review and refine AI output. Remove unnecessary code and verify it still works after your edits.
    • Process sharing — Explain how you used the AI so others can learn.

    Examples of good disclosures:

    "I used Claude Code to implement the component, prompting it to follow the pattern in ComponentX. I reviewed the generated code, removed unnecessary error handling, and verified the tests pass."

    "I brainstormed the approach with Gemini, then had it write failing tests for the feature. After reviewing the tests, I used Claude Code to generate the implementation. I refactored the output to reduce verbosity and ran the full test suite."

Also check that issue requirements are satisfied & you ran pre-commit locally.

Pull requests that don't follow the guidelines will be closed.

Reviewer assignment can take up to 2 weeks.

@AlexVelezLl AlexVelezLl requested a review from rtibblesbot May 19, 2026 18:46
@learning-equality-bot
Copy link
Copy Markdown

📢✨ Before we assign a reviewer, we'll turn on @rtibblesbot to pre-review. Its comments are generated by an LLM, and should be evaluated accordingly.

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 11 acceptance criteria are met in the code. CI passing.

Visual verification: the PR has no screenshots and the dev server was not available to generate them — see blocking comment.

Findings:

  • blocking: No screenshots provided for a PR with significant visual changes (card layout, KSelect, background color, headers). Please add before/after screenshots showing the new card structure, question headers, and grey background on the Questions tab.
  • suggestion: CSS duplication between AssessmentEditor.vue and AssessmentItemPreview.vue — see inline comment.
  • suggestion: Fragile negative margin on hints divider — see inline comment.
  • praise: Defensive kindLabel null guard prevents translator crash on unknown item types — see inline comment.
  • praise: Re-querying DOM state after async operations in tests is a good improvement — see inline comment.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

…assessment components

Signed-off-by: Abhishek-Punhani <punhani.manavabhi@gmail.com>
@Abhishek-Punhani
Copy link
Copy Markdown
Member Author

image Screenshot from 2026-05-21 00-45-55

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior findings

Resolved:

  • No screenshots for a PR with significant visual changes (blocking) — screenshots added as PR comment; visual inspection below
  • CSS duplication between AssessmentItemPreview.vue and AssessmentEditor.vue (suggestion) — comment added explaining scoped-style constraint and future extraction path
  • Fragile -28px negative margin coupling in .hints-divider (suggestion) — comment added documenting the coupling with AssessmentEditor.vue

3/3 prior findings resolved. 0 re-raised.


CI: Build and deploy tests failing on the latest commit — "Build frontend assets" fails at the "Use Node.js" setup step (step 4 of 13) in 16 seconds. All functional checks pass (JS Tests, Python Tests, Linting, Container Build). This is a transient GitHub Actions infrastructure failure unrelated to the code changes. Please re-run the failing job before merging.

Visual inspection: Screenshots provided as PR comment. Screenshot 1 shows KPageContainer card structure, "Question 1 of 1" header with move controls, "Type" label above KSelect, and the grey Questions tab background — all consistent with the target design from issue #5912. Screenshot 2 shows the Answers and Hints sections. No preview-state screenshot provided, but the prior round's code review confirmed AssessmentItemPreview implements the correct structure.

Findings:

  • suggestion: Please re-run the failing "Build frontend assets" CI job before merging — it's a transient Node.js setup failure, not a code issue, but the check needs to be green.
  • praise: The documentation comments added in this commit are well-written — both the AssessmentItemPreview scoped-styles note and the AssessmentItemEditor margin-coupling note are accurate, specific, and include actionable forward-looking guidance ("when a shared QuestionCard component is eventually built, these are the styles to extract into it").
  • nitpick: Screenshots were added as a PR comment rather than to the PR description. The PR template asks for screenshots in the description; they're easier to find there during review than in the comment thread.

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

  • Retrieved prior bot reviews deterministically via the GitHub API
  • Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
  • Only raised NEW findings for newly introduced code
  • Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
  • Checked CI status and linked issue acceptance criteria

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Assessment Editor layout and card structure

2 participants