Skip to content

Change lms config editor to be closer to existing settings behavior - #25

Draft
kurtnordstrom wants to merge 2 commits into
mainfrom
lmsconfig-ui-tweak
Draft

Change lms config editor to be closer to existing settings behavior#25
kurtnordstrom wants to merge 2 commits into
mainfrom
lmsconfig-ui-tweak

Conversation

@kurtnordstrom

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

Per-entry state reset on id change doesn’t clear savingFields, which can leave controls disabled after navigating to a different entry.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Updates the LMS configuration editor UI to match existing “settings-style” behavior by making fields read-only until explicitly edited, then saved/canceled per field.

Changes:

  • Replaces the row/column + always-editable inputs layout with per-field Card sections that support Edit/Save and Cancel.
  • Adds per-field editing state management (including preserving in-progress edits across entry refreshes).
  • Introduces component-scoped CSS for the new inline edit/cancel layout.
File summaries
File Description
ui-rsdir/src/components/LMSConfigEditor.js Implements per-field Card-based view/edit/save/cancel behavior and refines state handling for edits/saves/errors.
ui-rsdir/src/components/LMSConfigEditor.css Adds flex layout styles for the inline editor row (input + cancel button).
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread ui-rsdir/src/components/LMSConfigEditor.js
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The updated input rendering removes label props from TextField/Select, which is a likely accessibility/UX regression compared to established usage in this codebase.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

ui-rsdir/src/components/LMSConfigEditor.js:222

  • TextField/Select inputs no longer receive a label prop (only an aria-label). In this codebase, fields are consistently rendered with a visible label (and typically the required indicator) via the component’s label prop (e.g., SymbolsField.js / AddressesField.js). Consider restoring label here so the editor preserves expected accessibility/UX for required fields.
    const commonProps = {
      'aria-label': intl.formatMessage({ id: fieldLabelId(fieldName), defaultMessage: fieldName }),
      id: `lms-config-${fieldName}`,
      error: fieldErrors[fieldName],
      onChange: handleChange(fieldName),
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants