Actually pass showClassNames to the classroom's ArticlePreview - #1236
Merged
Merged
Conversation
A student in more than one class gets one merged classroom list, and until now it was a pile of texts with no way to tell whose lesson was whose. Each text now carries its class as a tag in the metadata strip, suppressed for the 2,484 students who have exactly one class and would see the same answer on every row. ArticlePreview renders two layouts and both have their own MetaStrip, so the tags are built once next to the other tag slots rather than inside either branch. Putting them in only one is a silent no-op for the classroom, which is exactly what happened first time. The class page also tells the teacher when it holds texts in a language its students are not learning -- those texts are invisible to the whole class, and the teacher is the only person who can do anything about it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-ups. - `cohort.language` is null for a class whose language was never set, and every language then failed the "is this off-language?" test, so the banner claimed the whole list was unreachable and left a hole in its own sentence where the language name should be. Both halves were false: the student filter compares the *article's* language to the *student's* learned language and never consults the class's. Six such classes exist; none holds a text yet, which is the only reason this was not already visible. - The classroom waited for getStudent before choosing an empty-state message but not before rendering the list, so the class tags -- which need to know whether the student has more than one class -- rendered absent and then appeared when that request landed. It waits in both paths now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The cohort id is a number in every payload now (api#719), but useParams() always hands back a string, so the two places that matched one against the other were relying on the API's odd stringification to work at all. Adds the regression this closes: a class shared during a session (ids from /cohorts_info) and the same class after a reload (ids from /teacher_texts) used to build two separate filter chips. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tags shipped dead. ClassroomArticles computed inMoreThanOneClass and never handed it to ArticlePreview, so from_classes was fetched, serialised, and dropped on the floor. Two things let that through, both mine: - The edit that was meant to add the prop was a string replace against a call site that no longer existed -- master had since replaced dontShowSourceIcon with a browsingModeProps spread. Every other replace in that script asserted its match; this one did not, so it silently changed nothing. - The scaffold I checked it with rendered ArticlePreview directly, passing showClassNames itself. It proved the component worked and never touched the one seam that was broken. Verified through ClassroomArticles this time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for voluble-nougat-015dd1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The class tags from #1235 shipped dead.
ClassroomArticlescomputedinMoreThanOneClassand then never handed it toArticlePreview, sofrom_classeswas fetched from the API, serialised, and dropped on the floor.One line to fix. Two process failures behind it, both mine:
A silent no-op replace. The edit meant to add the prop matched against
<ArticlePreview key article dontShowSourceIcon />— but master had since changed that call site to spreadbrowsingModeProps(...). Every other replacement in that script asserted its match; this one didn't, so it changed nothing and reported success.A scaffold that tested the wrong thing. I verified with a preview that rendered
ArticlePreviewdirectly and passedshowClassNames={true}itself. That proved the component rendered tags — while the only seam that was broken sat one level up, untouched. This time the scaffold renders the realClassroomArticleswith a fake API, and the tags appear through the actual path.Needs a web redeploy; the API side is already live and correct.