CLUE-343 Custom Labels for Problem Hierarchy#2736
CLUE-343 Custom Labels for Problem Hierarchy#2736tealefristoe wants to merge 32 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2736 +/- ##
==========================================
- Coverage 86.81% 86.79% -0.02%
==========================================
Files 829 828 -1
Lines 44549 44568 +19
Branches 11456 11449 -7
==========================================
+ Hits 38675 38685 +10
- Misses 5525 5534 +9
Partials 349 349
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
collaborative-learning
|
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
clue-343-naming-problems
|
| Run status |
|
| Run duration | 03m 01s |
| Commit |
|
| Committer | Teale Fristoe |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
|
@scytacki I've asked for your review, but feel free to ignore most of this PR. I mostly want your opinion on new translation keys. |
emcelroy
left a comment
There was a problem hiding this comment.
This is looking really good 👍 I left comments/questions about some minor things. There's at least one lint warning that should be addressed, and it looks like there are conflicts in src/components/document/document.tsx. Also, I think it'd be good to add a note to the PR description about how the use of tagPrompt in existing clue-curriculum code needs to be updated.
scytacki
left a comment
There was a problem hiding this comment.
I'd guess any changes to keys would break the units where Leslie has customized terms. But since we haven't released this official yet, now seems like the best time to make the change.
The capitalization in the translation keys is starting to bother me. I suggest starting all the keys with lowercase.
I also think we should scope the Problem, Investigation, and Unit keys. These keys are all specific to the curriculum structure or hierarchy. "Problem" is a little tricky because it we also use it at least in the code for ProblemDocument, but I don't think we expose that anywhere intentional. And it kind of makes sense that Problem Document means a document associated with the lowest curriculum level.
How about a scope of contentLevel, so these keys would be contentLevel.problem, contentLevel.investigation, contentLevel.unit.
I think the approach of defining explicit plural keys is a good way to go. 👍
|
@emcelroy I ended up making more changes than I was hoping after your last review, so your followup review will probably be bigger than you were expecting. Sorry. Here are the high level changes that I made:
|
emcelroy
left a comment
There was a problem hiding this comment.
Looks great 👍 Nice changes. The only comments I left are very minor. Also, this is a real nitpick, sorry, but I did wonder about the use of Term versus Word in some of the variable names (e.g., problemTerm and problemWord). It seems like at least in some cases it was an arbitrary choice which leads to some inconsistency. Not a big deal though if you leave those as is (especially if there's a good reason for it that I'm missing).
src/components/chat/comment-card.tsx
Outdated
| //if tagPrompt was posted to Firestore - for ex: SAS unit (where tagPrompt = "Select Student Strategy") | ||
| //our comment.tags should be [""] | ||
| const isTagPrompt = (comment.tags && comment.tags[0] === "") || (comment.tags === undefined); | ||
| //our comment.tags[0] should be [""] |
There was a problem hiding this comment.
Should the comment say `//our comment.tags[0] should be ""?
| } | ||
| else { | ||
| const groupTerm = translate("studentGroup"); | ||
| const groupTerm = groupWord; |
There was a problem hiding this comment.
This seems unnecessary. Can't you simply use groupWord in the next line?
Jira story: https://concord-consortium.atlassian.net/browse/CLUE-343
This PR adds a few new features:
document-type-collection.tsx). This was a specific request in the Jira story.I also refactored some code as part of this PR:
tagPrompthas mostly been removed from the codebase. Now, the override for "Strategy" will be used as the tag prompt, and Strategy will only appear in the "Sort by" dropdown if it has been overridden.translation-types.ts. Much of the content of this was sort specific, which I moved tosort-utils.ts. I moved the rest of the contents totranslate.ts, which is nice because it's now possible to import these on one line withtranslate.TERM_METADATAhas been moved intoterm-overrides-settings.tsx, since that's the only place where it's used.term-overrides-settings.tsx, rather than their translation keys.UI changes to
sort-work-settings.tsx:*if it differed from the type.