-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat(settings): enable Pad-wide Settings by default; fix misleading modal title #7679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,11 +127,7 @@ | |
| <!-------------------------------------------------------------> | ||
|
|
||
| <div id="settings" class="popup" role="dialog" aria-modal="true" aria-labelledby="settings-title"><div class="popup-content"> | ||
| <% if (settings.enablePadWideSettings) { %> | ||
| <h1 id="settings-title" data-l10n-id="pad.settings.title">Settings</h1> | ||
| <% } else { %> | ||
| <h1 id="settings-title" data-l10n-id="pad.settings.padSettings">Pad Settings</h1> | ||
| <% } %> | ||
| <div class="settings-sections"> | ||
|
Comment on lines
129
to
131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. No test for modal h1 The PR fixes the settings modal heading logic by removing the conditional title, but it does not add a regression test that would fail if the old misleading heading behavior returned. This makes the UI bug fix unverifiable and prone to regression. Agent Prompt
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 912da32. Added |
||
| <div id="user-settings-section" class="settings-section"> | ||
| <% e.begin_block("mySettings"); %> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| 'use strict'; | ||
|
|
||
| import {MapArrayType} from '../../../node/types/MapType'; | ||
| import settings from '../../../node/utils/Settings'; | ||
|
|
||
| const assert = require('assert').strict; | ||
| const common = require('../common'); | ||
|
|
||
| // Regression coverage for the settings modal title. With | ||
| // `enablePadWideSettings: false` the template used to render | ||
| // `data-l10n-id="pad.settings.padSettings"` ("Pad-wide Settings") for every | ||
| // user, even though no pad-wide controls were rendered in that mode. The fix | ||
| // removes the conditional and always uses `pad.settings.title` ("Settings"). | ||
| describe(__filename, function () { | ||
| this.timeout(30000); | ||
| let agent: any; | ||
| const backup: MapArrayType<any> = {}; | ||
|
|
||
| before(async function () { agent = await common.init(); }); | ||
|
|
||
| beforeEach(async function () { | ||
| backup.enablePadWideSettings = settings.enablePadWideSettings; | ||
| }); | ||
|
|
||
| afterEach(async function () { | ||
| settings.enablePadWideSettings = backup.enablePadWideSettings; | ||
| }); | ||
|
|
||
| const titleH1 = (html: string): string | null => { | ||
| const m = html.match(/<h1\s+id="settings-title"[^>]*data-l10n-id="([^"]+)"/); | ||
| return m ? m[1] : null; | ||
| }; | ||
|
|
||
| it('uses pad.settings.title with the feature enabled', async function () { | ||
| settings.enablePadWideSettings = true; | ||
| const res = await agent.get('/p/headingTest').expect(200); | ||
| assert.equal(titleH1(res.text), 'pad.settings.title'); | ||
| }); | ||
|
|
||
| it('uses pad.settings.title with the feature disabled (no misleading "Pad-wide" label)', async function () { | ||
| settings.enablePadWideSettings = false; | ||
| const res = await agent.get('/p/headingTest').expect(200); | ||
| assert.equal(titleH1(res.text), 'pad.settings.title'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. enablepadwidesettings default enabled
📘 Rule violation☼ ReliabilityAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation toolsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional. The maintainer (@JohnMcLear) explicitly approved flipping this default in conversation prior to opening the PR — see the PR description under "Why this is a behavior change worth taking." The creator-only
canEditPadSettingsserver gate is unchanged, so this widens UI defaults but does not widen access. Operators who want the legacy single-section modal can still setenablePadWideSettings: false; the comment in both shipped settings files documents that path.