feat: redesign Advanced Settings page with grouped sections and type-specific inputs#3019
feat: redesign Advanced Settings page with grouped sections and type-specific inputs#3019SantiagoSuHe wants to merge 10 commits intoopenedx:masterfrom
Conversation
…specific inputs Redesigns the Advanced Settings page in Studio for course authors. Instead of a flat alphabetical list of ~82 settings, settings are now organized into collapsible sections by functional category, with type-aware input components and a search/filter bar. New components: - SettingsFilters: search bar with collapse/expand all and deprecated settings toggle - SettingsSection: collapsible section using Paragon Collapsible.Advanced, renders settings grouped by category; supports subcategory grouping (Content Blocks) - CourseDisplayOverrides: dedicated UI for display-override fields with enable/disable toggle and blocked state messaging - BooleanInput, StringInput, NumberInput, EnumInput, JsonInput: type-specific inputs New data layer: - settingsCategories.ts: maps all settings keys to 13 categories with display order, exported constants, and subcategory map for Content Blocks - fieldTypes.ts: getFieldType() and serializeValue() utilities; ENUM_OPTIONS definitions - fieldTypeMessages.ts: i18n messages for enum labels and field placeholders - types.ts: shared TypeScript interfaces (SettingData, SettingEntry, SetEditedSettings) Modified: - SettingCard.tsx: renders type-specific input components instead of a single generic field; passes displayName as aria-label for accessibility - AdvancedSettings.tsx: groups settings via groupSettingsByCategory(), renders one SettingsSection per category; data layer (React Query, API calls, selectors) untouched - AdvancedSettings.scss: styles for section headers, collapsible triggers, subcategory labels Not modified: Redux store, API layer, thunks, selectors, routing.
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @SantiagoSuHe! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
…s.ts
- Run dprint fmt on all new advanced-settings files to comply with the
project's code formatting configuration
- Fix i18n extraction error in src/generic/create-or-rerun-course/messages.ts:
replace template literal defaultMessage with FormatJS {maxLength} placeholder;
update hooks.jsx to pass maxLength value at runtime; update test assertion
Replace incorrect `as [string, object][]` cast with `as SettingEntry[]` so the type checker can verify the prop against SettingsSection's expected type. Also adds the missing `SettingEntry` type import. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3019 +/- ##
==========================================
+ Coverage 95.47% 95.51% +0.03%
==========================================
Files 1383 1407 +24
Lines 32657 33232 +575
Branches 7259 7675 +416
==========================================
+ Hits 31180 31742 +562
+ Misses 1421 1420 -1
- Partials 56 70 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Wow, I didn't know this was happening and I'm so happy to see this PR! Could you please add a screenshot or two to the PR description? And let me know when you'd like a review! |
|
Hi @bradenmacdonald thanks for your comment. |
- SettingCard: add tests for boolean, number, enum and JSON input rendering; handleImmediateChange for boolean/enum; catch branch in getDisplayValue when JSON.parse throws - SettingsSection: add tests for forceOpen re-expand behaviour and CATEGORY_GENERAL_SETTING branch (CourseDisplayOverrides rendering) - SettingsFilters: add test for onClear callback via form reset - BooleanInput: add test for displayName || name aria-label fallback - EnumInput: add tests for onBlur, displayName fallback, null value and unknown field name - StringInput: add tests for default value param, displayName fallback and null value - NumberInput: add tests for default value param and displayName fallback - JsonInput: capture updateListener callback; add tests for user-driven onChange and programmatic dispatch suppression Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…uotes) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Sorry for the slow response here, I haven't had time to fully test or review this in depth. I just had some high level questions about the approach for grouping the settings and the UI controls.
| * Field type classification for all Advanced Settings. | ||
| * Type is detected at runtime by getFieldType(key, value) based on typeof value + this enum map. |
There was a problem hiding this comment.
Can't the backend provide the type information via the API? I don't like having to hard-code so much on the frontend.
And if we are hard-coding it on the frontend, what do you think about combining the "categories" and "field types" information?
You have
categories = {
displayName: 'General Setting',
...
}
fieldTypes = {
boolean: ...,
enum: ...,
}
Wouldn't it be easier to understand as this?
const advancedSettings = [
{
name: "General Settings",
fields: [
{code: "displayName", type: "string", label: messages.displayNameField},
{code: "courseEditMethod", type: "enum", values: [
{ value: 'Studio', label: 'Studio' },
{ value: 'XML', label: 'XML' },
], label: messages.courseEditMethodField},
],
}
];
But better yet, can't we get the backend to send us this data ^ ?
There was a problem hiding this comment.
We could - one intersting note here is that if we continue with other feature ideas -- we could migrate many / most of these categories away from this page in future releases, so not opposed to formalizing the categories but technically we have a feature idea to migrate all of these categories except for "Other"
There was a problem hiding this comment.
Hi @bradenmacdonald Thanks for the feedback.
We also liked your idea of combining the categories and field types info, we went ahead and implemented that. Both files now derive all their data from a single settingsConfig.ts that groups each field with its category, type, and enum options in one place.
The reason we hardcoded the files on the frontend is that we intentionally wanted to keep the scope of this PR to frontend only changes, no backend modifications required to ship this redesign.
That said, we're also aware that many of these settings will eventually be migrated out of Advanced Settings into more appropriate places in Studio, so some of these categories may become irrelevant over time anyway.
We're fully open to doing this the right way if you think a backend driven approach is the better long term solution. If that's the direction, we're happy to open a PR in the platform repo to expose type/category metadata via the API. Just let us know how you'd like to proceed and we'll follow your lead.
| @@ -0,0 +1,25 @@ | |||
| import { Form } from '@openedx/paragon'; | |||
|
|
|||
There was a problem hiding this comment.
All these new fields BooleanInput, EnumInput, JsonInput etc. are currently here in src/advanced-settings/setting-card/inputs/...`
Do you think we might want to re-use these components in other contexts like the component editor, library settings, or other places? Or are they really specific to the advanced settings UI?
There was a problem hiding this comment.
If a reuse opportunity comes up, we can extract them to a shared location at that point. For now, keeping them here avoids premature abstraction.
|
@brian-smith-tcril @arbrandes Just flagging this PR - I'm not sure if it's possible for either of you to work with @SantiagoSuHe and get it over the line for Verawood. I don't think I'll have much time to help between now and the cut. If so, that would be amazing, but it's not a release blocker or anything afaik. |
PKulkoRaccoonGang
left a comment
PKulkoRaccoonGang
left a comment
There was a problem hiding this comment.
Thanks for this PR - this is a significant improvement over the previous version. I've left a few comments, including one question regarding the empty search alert.
Have you considered showing an alert indicating that no results were found?
| margin-bottom: 1.75rem; | ||
| // Section is now a Card; individual setting rows replace per-setting Cards | ||
| .settings-section { | ||
| overflow: hidden; |
There was a problem hiding this comment.
Do we really need to have overflow: hidden; on this element?
| font-size: 1.125rem; | ||
| } | ||
| .sub-header { | ||
| padding-bottom: 1.5rem; |
There was a problem hiding this comment.
Can we use Paragon Utility Classes instead of new styles?
| const intl = useIntl(); | ||
|
|
||
| return ( | ||
| <div className="settings-filters d-flex align-items-center gap-3 mb-4"> |
There was a problem hiding this comment.
Could we use the Paragon Stack component here and remove the extra styles?
| gap: .75rem; | ||
|
|
||
| .settings-filters-search { | ||
| width: 280px; |
There was a problem hiding this comment.
Do we really need to hardcode the width for this element? I tried removing it and didn't notice any difference.
If there's indeed a need for it, we can use Paragon utility classes (e.g. w-25 or w-50)
| return ( | ||
| <Card className="settings-section mb-4"> | ||
| <Collapsible.Advanced open={isOpen} onToggle={setIsOpen}> | ||
| <Collapsible.Trigger className="settings-section-trigger w-100 p-0 border-0 bg-transparent text-left"> |
There was a problem hiding this comment.
Do we really need all these CSS classes? I tried removing them and didn't notice any difference.
| import NumberInput from './NumberInput'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const renderInput = (props: Record<string, any> = {}) => |
There was a problem hiding this comment.
Can we use NumberInputProps?
| size="sm" | ||
| /> | ||
| </div> | ||
| <Button |
There was a problem hiding this comment.
The Button component already has an iconAfter prop should we use it here to render the icon instead?
| src={InfoOutline} | ||
| iconAs={Icon} | ||
| alt={intl.formatMessage(messages.courseDisplayOverridesInfoAlt)} | ||
| variant="primary" |
There was a problem hiding this comment.
We can remove this. "Primary" is the default variant for IconButton.
| alt={intl.formatMessage(messages.courseDisplayOverridesInfoAlt)} | ||
| variant="primary" | ||
| size="sm" | ||
| className="flex-shrink-0" |
There was a problem hiding this comment.
Do we really need this CSS class?
| ]; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const renderSection = (props: Record<string, any> = {}) => |
There was a problem hiding this comment.
Can we use SettingsSectionProps?
…onald - Unify settingsCategories + fieldTypes into single settingsConfig.ts source of truth - Replace custom SCSS with Paragon utility classes (mx-1, my-0, font-weight-bold, etc.) - Use Paragon Stack component in SettingsFilters instead of manual flex layout - Use Form.Control as="select" in EnumInput instead of raw <select> - Use Button iconBefore prop instead of manual Icon child in SettingsFilters - Remove redundant variant="primary" from IconButton (Paragon default) - Remove unnecessary CSS classes confirmed to have no visual effect - Add barrel file inputs/index.ts and import all inputs from it in SettingCard - Migrate fireEvent → userEvent in all new test files - Export and use typed interfaces (BooleanInputProps, EnumInputProps, NumberInputProps, SettingsSectionProps) in tests instead of Record<string, any> - Add headerClassName prop to SubHeader for injecting utility classes - Show Alert when search returns no results - Add Badge mx-1 shorthand and other margin shorthands throughout Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
arbrandes
left a comment
arbrandes
left a comment
There was a problem hiding this comment.
Let me start by saying that this looks and works great. It will be a much needed improvement, once it lands. BUT...
This introduces coupling where before there was none. Meaning, the settings metadata now lives in the frontend, hardcoded against the set of ~82 keys in settingsConfig.ts. That's a lot.
There are two concretely bad consequences of this coupling (one of them very bad):
- Drift. When edx-platform's set of Advanced Settings changes, whether because a new field is added or an existing one renamed, the frontend silently degrades: the unknown key lands in "Other" with type-by-value-shape detection, obviating the categorization this PR introduces.
- Data loss. If the backend ever adds a new
showanswervalue, the dropdown won't include it - and worse, if a course already has that value set, the<select>won't render it as the current selection. Silent data loss on save is possible. This is a new risk: pre-PR the user typed JSON directly, so any backend value round-tripped fine.
If it weren't for the second consequence, I might be convinced to merge this for the UX improvement alone. But in the end this implementation is not only just nice-to-have, but also obsolete on inception: it seems the real plan is to relocate the settings (and, presumably, guard them) to more logical places in Studio. Though it's likely that, as somebody apparently suggested, we will
"keep Advanced Settings as a lightweight space for experimental or rarely used configurations, since it allows the addition of new or temporary options without requiring new front-end development"
That means we will continue to need a generic solution, with no coupling. Might as well solve the problem now. 🤷🏼
Apologies for the rejection, particularly since this is such an obvious UX improvement. I'll be happy to continue reviewing once it's refactored so that the source of truth for the metadata comes entirely from the backend, though.
|
Hi @arbrandes, thanks for the review and the clear explanation. |
7 participants
Description
Redesigns the Advanced Settings page in Studio for course authors. Instead of a flat alphabetical list of ~82 settings, they are now organized into collapsible sections by functional category, with type-aware input components and a search/filter bar.
Screen.Recording.2026-04-23.at.5.00.17.PM.mov
User role affected: Course Author
What changed
New components:
SettingsFilters— search bar with collapse/expand all, and deprecated settings toggleSettingsSection— collapsible section using Paragon'sCollapsible.Advanced, renders settings grouped by category; supports subcategory grouping (used by Content Blocks → Settings / Problems / Video)CourseDisplayOverrides— dedicated UI for the three display-override fields with enable/disable toggle and blocked state messagingBooleanInput,StringInput,NumberInput,EnumInput,JsonInput— type-specific input components used bySettingCardNew data layer:
settingsCategories.ts— maps all settings keys to 13 categories with display order, exported constants, and subcategory map for Content BlocksfieldTypes.ts—getFieldType()andserializeValue()utilities;ENUM_OPTIONSandFIELD_PLACEHOLDER_MESSAGESdefinitionsfieldTypeMessages.ts— i18n messages for enum labels and field placeholderstypes.ts— shared TypeScript interfacesModified:
SettingCard.tsx— now renders type-specific input components instead of a single generic field; passesdisplayNameasaria-labelAdvancedSettings.tsx— groups settings viagroupSettingsByCategory(), renders oneSettingsSectionper category; data layer (React Query, API calls, selectors) is untouchedAdvancedSettings.scss— styles for section headers, collapsible triggers, and subcategory labelsNot modified: Redux store, API layer, thunks, selectors, routing.
Categories
13 sections: General Setting · Content Blocks · Grading · Schedule · Certificates · Enrollment Page · Pages & Resources · Special Exams · Mobile · Instructors · Legacy Discussion · Libraries · Other
Supporting information
This PR is motivated by two related initiatives:
Schema's internal proposal — Relocating Advanced Settings (https://openedx.atlassian.net/wiki/spaces/COMM/pages/5236785155/Relocating+Advanced+Settings): we've been thinking about migrating the ~70 Advanced Settings in Studio into clearer homes across Studio's configuration, reducing reliance on the Advanced Settings page for tools that have matured. That broader migration is still the long-term direction.
Discourse — Modernize UI options for Advanced Settings (https://discuss.openedx.org/t/modernize-ui-options-for-advanced-settings/17269): rather than starting with the larger migration, we think a better first step is to improve the usability of the page as it exists today. That's what this PR does. Moving individual settings to better homes can follow incrementally.
This is our first direct code contribution to the Open edX project from Schema, assisted by Claude Code. We're actively looking for feedback to improve our future contributions.
Testing instructions
/course/{course_id}/settings/advanced).showanswer) and verify the dropdown renders.Other information
No new Redux state. No DB changes. No migrations. No feature flag needed (UI-only change within the existing
advanced-settingssubsystem).../relative imports remain in some new components (SettingsSection.tsx,CourseDisplayOverrides.tsx). Migrating to@srcaliases is a follow-up item.Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(initializeMocks).messages.tsfiles have adescription.../in import paths. → Some../imports remain in new components — follow-up item.