Shorten subject names to prefix before hyphen for charts and standards - #116
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the shortenName function to use hyphen-based splitting for subject names instead of a hardcoded map. Feedback indicates a bug in null handling where null values are converted to the string 'null', and a regression where specific manual mappings for subjects like '英語文' were removed, leading to longer labels in the UI. A suggestion was provided to restore these mappings and improve input validation.
| export function shortenName(name = '') { | ||
| const cleanedName = cleanSubjectName(String(name)); | ||
| const [baseName] = cleanedName.split('-'); | ||
| return baseName.trim() || cleanedName; | ||
| } |
There was a problem hiding this comment.
The updated shortenName function introduces two issues:
- Null Handling: If
nameisnull,String(name)returns the string'null', which is then processed and returned. The previous implementation correctly returnednull(whichescapeHTMLhandles as an empty string). Using a truthiness check or explicit null check is safer. - Shortening Regression: The manual mappings for
'英語文' -> '英文'and'公民與社會' -> '公民'were removed. This results in these subject names appearing longer in the UI, which contradicts the primary goal of shortening subject labels for better display in charts and tables.
Consider restoring these specific mappings and adding a robust null check.
export function shortenName(name) {
if (!name) return '';
const cleanedName = cleanSubjectName(String(name));
const [baseName] = cleanedName.split('-');
const result = baseName.trim() || cleanedName;
// Maintain manual mappings for subjects that don't follow the hyphen pattern
const manualMappings = {
'英語文': '英文',
'公民與社會': '公民'
};
return manualMappings[result] || result;
}There was a problem hiding this comment.
Pull request overview
This PR updates how subject names are shortened across the frontend so hyphenated elective subject labels display using only the prefix before the first -, improving consistency in charts and standards displays.
Changes:
- Replaced the mapping-based
shortenNamewith logic that cleans subject names and returns the text before the first-. - Updated frontend tests to validate the new hyphen-prefix shortening behavior and ensure non-hyphenated names remain unchanged.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
frontend/dashboard.js |
Reworks shortenName to derive the shortened label from the pre-hyphen prefix after cleaning. |
tests/frontend/dashboard.test.js |
Updates unit tests to reflect the new shortenName behavior for hyphenated vs. non-hyphenated names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function shortenName(name = '') { | ||
| const cleanedName = cleanSubjectName(String(name)); |
There was a problem hiding this comment.
shortenName currently does String(name), so shortenName(null) becomes the literal string 'null' (and undefined becomes 'undefined'). Since SubjectName comes from the backend via subject.get('SubjectName'), null values are possible and would display incorrectly in the UI. Consider normalizing with String(name ?? '') (or returning '' for nullish values) before cleaning/splitting.
| export function shortenName(name = '') { | |
| const cleanedName = cleanSubjectName(String(name)); | |
| export function shortenName(name) { | |
| const normalizedName = name ?? ''; | |
| const cleanedName = cleanSubjectName(String(normalizedName)); |
| it('should return original name when no hyphen exists', () => { | ||
| assert.equal(shortenName('數學A'), '數學A'); | ||
| assert.equal(shortenName('英語文'), '英語文'); | ||
| }); |
There was a problem hiding this comment.
The updated shortenName behavior includes cleaning <br/> (via cleanSubjectName) and new handling for non-string inputs (default param). The tests only cover hyphen splitting for normal strings; please add coverage for a name containing <br/> and for nullish inputs (e.g., null/undefined) to prevent regressions and to lock in the intended output.
| }); | |
| }); | |
| it('should clean <br/> tags before shortening', () => { | |
| assert.equal(shortenName('選修化學<br/>-物質構造與反應速率'), '選修化學'); | |
| assert.equal(shortenName('英語文<br/>'), '英語文'); | |
| }); | |
| it('should handle nullish inputs using the default value', () => { | |
| assert.equal(shortenName(), ''); | |
| assert.equal(shortenName(undefined), ''); | |
| assert.equal(shortenName(null), ''); | |
| }); |
Motivation
-so elective names like選修物理-力學二與熱學display as選修物理and選修化學-物質構造與反應速率display as選修化學in all UI places that show subject names.Description
shortenNameinfrontend/dashboard.jswith a function that first cleans<br/>then returns the substring before the first-(shortenName(name)).frontend/charts.js(personal vs. class average and score comparison) and the class standards table infrontend/dashboard.jsnow automatically use the updatedshortenNamebehavior.tests/frontend/dashboard.test.jsto validate the new behavior for hyphenated elective names and to ensure names without-remain unchanged.Testing
npm testand all frontend tests passed (all tests green).tests/frontend/dashboard.test.jsverify選修物理-力學二與熱學 -> 選修物理,選修化學-物質構造與反應速率 -> 選修化學, and unchanged output for names without a hyphen.Codex Task