-
Notifications
You must be signed in to change notification settings - Fork 57
feat: improve UX in properties editor#1620
feat: improve UX in properties editor#1620anteeek wants to merge 2 commits intoSofie-Automation:mainfrom
Conversation
About the Contributor
This PR is made on behalf of the BBC
Type of Contribution
This is a:
Feature
Current Behavior
Close button ("x") is positioned incorrectly
"Restore from NRCS" icon is misaligned and too close to text
Properties Panel goes blank when piece ID changes after edit (should close instead)
Empty Properties Panel opens for items with no editable properties
Properties Panel doesn't open when notification panels are visible
Clicking Properties icon when notifications are open closes Properties instead of closing notifications
Sidebar has wrong background color when panel is open
Unnecessary spacing between Segments and Properties Panel
Operations lack proper margins
New Behavior
Close button is properly positioned
"Restore from NRCS" icon is correctly aligned with appropriate spacing
Properties Panel closes automatically when piece ID changes
Properties Panel doesn't open for items without editable properties
Properties Panel opens and auto-closes notifications when needed
Properties icon closes notifications and shows Properties Panel
Sidebar displays correct background color
Optimized spacing between Segments and Properties Panel
Operations display with appropriate margins
Testing
- I have added one or more unit tests for this PR
- I have updated the relevant unit tests
- No unit test changes are needed for this PR
Affected areas
Properties editor in rundown view
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status
- PR is ready to be reviewed.
- The functionality has been tested by the author.
- Relevant unit tests has been added / updated.
- Relevant documentation (code comments, system documentation) has been added / updated.
WalkthroughAdded a CLOSE_NOTIFICATIONS event to coordinate notification center visibility with properties panel. Implemented conditional rendering to display either the notification center or properties panel based on user selection state. Added Changes
Sequence Diagram(s)
sequenceDiagram
participant User participant SourceLayerItem as SourceLayerItem (double-click) participant EventBus as RundownViewEventBus participant RundownView participant Panels as Notification/Properties Panels User->>SourceLayerItem: Double-click piece alt Content is not editable SourceLayerItem-->>User: Action blocked else Content is editable SourceLayerItem->>EventBus: Emit CLOSE_NOTIFICATIONS EventBus->>RundownView: CLOSE_NOTIFICATIONS event RundownView->>RundownView: Set isNotificationsCenterOpen = undefined RundownView->>Panels: Re-render with selection Panels-->>User: Show PropertiesPanel (hide NotificationCenterPanel) end Estimated code review effort3 (Moderate) | ~22 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks | 2 | 1Failed checks (1 warning)
Passed checks (2 passed)
Tip: You can configure your own custom pre-merge checks in the settings. Finishing TouchesGenerate unit tests (beta)
Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report Patch coverage is
Thoughts on this report? Let us know! |
nytamin
left a comment
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.
@anteeek Since this PR affects the GUI, could you add a screenshot in the description to better describe the changes?
b3ebcf3 to
d43cf78
Compare
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.
Actionable comments posted: 6
Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/webui/src/client/styles/propertiesPanel.scss`:
- Around line 236-244: The .propertiespanel-pop-up__button rule inside
.propertiespanel-pop-up__contents is missing display:flex so its gap: 0.5em has
no effect and Stylelint flags the blank line; update the
.propertiespanel-pop-up__button selector (same block shown) to include display:
flex and remove the empty line before gap so the styles match the
.propertiespanel-pop-up__footer pattern and gap takes effect.
In `@packages/webui/src/client/styles/rundownView.scss`:
- Around line 212-221: Remove the extra blank line immediately before the
background declaration so the CSS/Sass rule has no empty line before the
"background:" property; locate the block containing the "background:
linear-gradient(...)" lines (the repeated linear-gradient entries using
$color-status-fatal) and delete the blank line above it to satisfy stylelint's
declaration-empty-line-before rule.
In `@packages/webui/src/client/ui/RundownView.tsx`:
- Around line 911-915: The onCloseNotifications handler currently only clears
component state (isNotificationsCenterOpen) which leaves the global singleton
flag NotificationCenter.isOpen out of sync; update onCloseNotifications to also
set NotificationCenter.isOpen = false when closing, and apply the same change to
any other auto-close paths (the handlers that open properties via the event bus
/ context menu that currently only clear isNotificationsCenterOpen) so the
global NotificationCenter.isOpen and the React state remain consistent.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsx`:
- Around line 1017-1025: Double-click handler in SegmentTimeline should close
notifications before changing selection: when enableUserEdits is true and
hasUserEditableContent(segment) passes, emit the same CLOSE_NOTIFICATIONS event
used in SourceLayerItem.tsx (using the app's notification/event emitter or
context) prior to calling selectElementContext.clearAndSetSelection({ type:
'segment', elementId: segment._id }), and only then proceed to set selection (or
clearAndSetSelection) so the right-hand panel isn't suppressed; update the
onDoubleClick branch around hasUserEditableContent, referencing the existing
symbols hasUserEditableContent, selectElementContext.clearAndSetSelection,
segment._id, and CLOSE_NOTIFICATIONS.
In `@packages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx`:
- Around line 25-29: The import of CoreUserEditingDefinition and
CoreUserEditingProperties currently points to
`@sofie-automation/corelib/src/dataModel/UserEditingDefinitions.js`; update that
import to use `@sofie-automation/corelib/dist/dataModel/UserEditingDefinit ions` so
it matches other imports (e.g., in RenderUserEditOperations.tsx and
SegmentContextMenu.tsx) and avoids breaking module resolution outside the
monorepo, keeping the imported symbol names (CoreUserEditingDefinition,
CoreUserEditingProperties) unchanged.
- Around line 394-408: The helper hasUserEditableContent currently treats any
userEditOperations as making the panel editable, which causes empty panels for
standalone ops like RETIME_PIECE; change it so it only returns true when
userEditProperties contains real content OR when userEditOperations includes at
least one operation that is actually rendered in the PropertiesPanel.
Concretely, update hasUserEditableContent to filter userEditOperations (e.g.
obj.userEditOperations.some(op => /* op has panel UI */ op.properties ||
op.fields || op.panel || allowedTypes.includes(op.type))) and ignore known
standalone ops such as RETIME_PIECE (see SourceLayerItem.tsx) so only ops that
provide panel fields cause a true result.
i Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca488cc6-7026-43b6-bdf3-7fff29c5a8e1
Files selected for processing (8)
packages/meteor-lib/src/triggers/RundownViewEventBus.tspackages/webui/src/client/styles/propertiesPanel.scsspackages/webui/src/client/styles/rundownView.scsspackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentTimeline.tsxpackages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsxpackages/webui/src/client/ui/UserEditOperations/PropertiesPanel.tsx
| .propertiespanel-pop-up__button { | ||
| // margin-top: 10px; | ||
| background: #636363; | ||
| padding: 10px; | ||
| gap: 10px; | ||
| border-radius: 5px; | ||
| border: 1px solid #7f7f7f; | ||
| color: #dfdfdf; | ||
|
|
||
| gap: 0.5em; | ||
|
|
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.
Analysis chain
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 564
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 985
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 1897
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 331
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 664
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 894
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 1826
Script executed:
Repository: Sofie-Automation/sofie-core
Length of output: 1379
Add display: flex and remove blank line before gap.
The .propertiespanel-pop-up__button selector nested inside .propertiespanel-pop-up__contents (line 236) is missing the display: flex property that exists in the earlier declaration under .propertiespanel-pop-up__footer (line 115). Without flex display, the gap: 0.5em; property has no effect. The blank line before gap is also a Stylelint error.
Suggested change
+ display: flex;
+ align-items: center;
background: `#636363`;
padding: 10px;
border-radius: 5px;
border: 1px solid `#7f7f7f`;
color: `#dfdfdf`;
-
gap: 0.5em;
Committable suggestion
!! IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .propertiespanel-pop-up__button { | |
| // margin-top: 10px; | |
| background: #636363; | |
| padding: 10px; | |
| gap: 10px; | |
| border-radius: 5px; | |
| border: 1px solid #7f7f7f; | |
| color: #dfdfdf; | |
| gap: 0.5em; | |
| .propertiespanel-pop-up__button { | |
| display: flex; | |
| align-items: center; | |
| background: `#636363`; | |
| padding: 10px; | |
| border-radius: 5px; | |
| border: 1px solid `#7f7f7f`; | |
| color: `#dfdfdf`; | |
| gap: 0.5em; |
Tools
Stylelint (17.4.0)
[error] 243-243: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/styles/propertiesPanel.scss` around lines 236 -
244, The .propertiespanel-pop-up__button rule inside
.propertiespanel-pop-up__contents is missing display:flex so its gap: 0.5em has
no effect and Stylelint flags the blank line; update the
.propertiespanel-pop-up__button selector (same block shown) to include display:
flex and remove the empty line before gap so the styles match the
.propertiespanel-pop-up__footer pattern and gap takes effect.
| background: linear-gradient( | ||
| -45deg, | ||
| $color-status-fatal 33%, | ||
| transparent 33%, | ||
| transparent 66%, | ||
| $color-status-fatal 66% | ||
| ), | ||
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | ||
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | ||
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%); |
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.
Remove the extra blank line before background.
Stylelint is still flagging this hunk with declaration-empty-line-before, so the stylesheet check will stay red until the newline is removed.
Suggested change
-
background: linear-gradient(
-45deg,
$color-status-fatal 33%,
Committable suggestion
!! IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background: linear-gradient( | |
| -45deg, | |
| $color-status-fatal 33%, | |
| transparent 33%, | |
| transparent 66%, | |
| $color-status-fatal 66% | |
| ), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%); | |
| right: 0; | |
| background: linear-gradient( | |
| -45deg, | |
| $color-status-fatal 33%, | |
| transparent 33%, | |
| transparent 66%, | |
| $color-status-fatal 66% | |
| ), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%), | |
| linear-gradient(-45deg, $color-status-fatal 33%, transparent 33%, transparent 66%, $color-status-fatal 66%); |
Tools
Stylelint (17.4.0)
[error] 212-221: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/styles/rundownView.scss` around lines 212 - 221,
Remove the extra blank line immediately before the background declaration so the
CSS/Sass rule has no empty line before the "background:" property; locate the
block containing the "background: linear-gradient(...)" lines (the repeated
linear-gradient entries using $color-status-fatal) and delete the blank line
above it to satisfy stylelint's declaration-empty-line-before rule.
| private onCloseNotifications = () => { | ||
| this.setState({ | ||
| isNotificationsCenterOpen: undefined, | ||
| }) | ||
| } |
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.
Sync the global notification-center flag when these paths auto-close it.
Lines 1200-1204 keep NotificationCenter.isOpen aligned with React state, but these new branches only clear isNotificationsCenterOpen. After opening properties through the event bus or the context menu, the singleton can still think the center is open, which makes later pop-up/toggle behavior drift out of sync.
Suggested change
+ NotificationCenter.isOpen = false
this.setState({
isNotificationsCenterOpen: undefined,
})
}
@@
onEditProps={(selection) => {
+ NotificationCenter.isOpen = false
this.setState({ isNotificationsCenterOpen: undefined })
selectionContext.clearAndSetSelection(selection)
}}
Also applies to: 1536-1539
Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/RundownView.tsx` around lines 911 - 915, The
onCloseNotifications handler currently only clears component state
(isNotificationsCenterOpen) which leaves the global singleton flag
NotificationCenter.isOpen out of sync; update onCloseNotifications to also set
NotificationCenter.isOpen = false when closing, and apply the same change to any
other auto-close paths (the handlers that open properties via the event bus /
context menu that currently only clear isNotificationsCenterOpen) so the global
NotificationCenter.isOpen and the React state remain consistent.
| onDoubleClick={() => { | ||
| if (this.props.studio.settings.enableUserEdits) { | ||
| if (!selectElementContext.isSelected(this.props.segment._id)) { | ||
| selectElementContext.clearAndSetSelection({ type: 'segment', elementId: this.props.segment._id }) | ||
| const segment = this.props.segment | ||
|
|
||
| const hasEditableContent = hasUserEditableContent(segment) | ||
| if (!hasEditableContent) return | ||
|
|
||
| if (!selectElementContext.isSelected(segment._id)) { | ||
| selectElementContext.clearAndSetSelection({ type: 'segment', elementId: segment._id }) |
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.
Close notifications before selecting a segment.
When the notification center is already open, this only changes the selection. RundownView.tsx now suppresses both side panels until notifications are closed (Lines 1477-1485 there), so a segment double-click can leave the right side blank instead of opening properties. SourceLayerItem.tsx already emits CLOSE_NOTIFICATIONS before piece selection; this path should do the same.
Suggested change
const segment = this.props.segment
const hasEditableContent = hasUserEditableContent(segment)
if (!hasEditableContent) return
if (!selectElementContext.isSelected(segment._id)) {
+ RundownViewEventBus.emit(RundownViewEvents.CLOSE_NOTIFICATIONS)
selectElementContext.clearAndSetSelection({ type: 'segment', elementId: segment._id })
} else {
selectElementContext.clearSelections()
}
Committable suggestion
!! IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onDoubleClick={() => { | |
| if (this.props.studio.settings.enableUserEdits) { | |
| if (!selectElementContext.isSelected(this.props.segment._id)) { | |
| selectElementContext.clearAndSetSelection({ type: 'segment', elementId: this.props.segment._id }) | |
| const segment = this.props.segment | |
| const hasEditableContent = hasUserEditableContent(segment) | |
| if (!hasEditableContent) return | |
| if (!selectElementContext.isSelected(segment._id)) { | |
| selectElementContext.clearAndSetSelection({ type: 'segment', elementId: segment._id }) | |
| onDoubleClick={() => { | |
| if (this.props.studio.settings.enableUserEdits) { | |
| const segment = this.props.segment | |
| const hasEditableContent = hasUserEditableContent(segment) | |
| if (!hasEditableContent) return | |
| if (!selectElementContext.isSelected(segment._id)) { | |
| RundownViewEventBus.emit(RundownViewEvents.CLOSE_NOTIFICATIONS) | |
| selectElementContext.clearAndSetSelection({ type: 'segment', elementId: segment._id }) | |
| } else { | |
| selectElementContext.clearSelections() | |
| } |
Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentTimeli ne.tsx` around
lines 1017 - 1025, Double-click handler in SegmentTimeline should close
notifications before changing selection: when enableUserEdits is true and
hasUserEditableContent(segment) passes, emit the same CLOSE_NOTIFICATIONS event
used in SourceLayerItem.tsx (using the app's notification/event emitter or
context) prior to calling selectElementContext.clearAndSetSelection({ type:
'segment', elementId: segment._id }), and only then proceed to set selection (or
clearAndSetSelection) so the right-hand panel isn't suppressed; update the
onDoubleClick branch around hasUserEditableContent, referencing the existing
symbols hasUserEditableContent, selectElementContext.clearAndSetSelection,
segment._id, and CLOSE_NOTIFICATIONS.
| export function hasUserEditableContent( | ||
| obj: | ||
| | ReadonlyDeep<{ | ||
| userEditOperations?: CoreUserEditingDefinition[] | ||
| userEditProperties?: CoreUserEditingProperties | ||
| }> | ||
| | undefined | ||
| ): boolean { | ||
| return !!( | ||
| obj?.userEditOperations?.length || | ||
| obj?.userEditProperties?.pieceTypeProperties || | ||
| obj?.userEditProperties?.globalProperties || | ||
| obj?.userEditProperties?.operations?.length | ||
| ) | ||
| } |
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.
Don't count non-panel operations as editable properties.
This helper now returns true for any userEditOperations. Line 129 of packages/webui/src/client/ui/SegmentTimeline/SourceLayerItem.tsx shows pieces can carry standalone ops like RETIME_PIECE, but PropertiesPanel still renders body content from userEditProperties only. Those items will still open an empty panel, which is exactly the regression this PR is trying to remove.
Suggested change
obj:
| ReadonlyDeep<{
userEditOperations?: CoreUserEditingDefinition[]
userEditProperties?: CoreUserEditingProperties
}>
| undefined
): boolean {
return !!(
- obj?.userEditOperations?.length ||
obj?.userEditProperties?.pieceTypeProperties ||
obj?.userEditProperties?.globalProperties ||
obj?.userEditProperties?.operations?.length
)
}
Committable suggestion
!! IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function hasUserEditableContent( | |
| obj: | |
| | ReadonlyDeep<{ | |
| userEditOperations?: CoreUserEditingDefinition[] | |
| userEditProperties?: CoreUserEditingProperties | |
| }> | |
| | undefined | |
| ): boolean { | |
| return !!( | |
| obj?.userEditOperations?.length || | |
| obj?.userEditProperties?.pieceTypeProperties || | |
| obj?.userEditProperties?.globalProperties || | |
| obj?.userEditProperties?.operations?.length | |
| ) | |
| } | |
| export function hasUserEditableContent( | |
| obj: | |
| | ReadonlyDeep<{ | |
| userEditOperations?: CoreUserEditingDefinition[] | |
| userEditProperties?: CoreUserEditingProperties | |
| }> | |
| | undefined | |
| ): boolean { | |
| return !!( | |
| obj?.userEditProperties?.pieceTypeProperties || | |
| obj?.userEditProperties?.globalProperties || | |
| obj?.userEditProperties?.operations?.length | |
| ) | |
| } |
Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/UserEditOperations/Properties Panel.tsx` around
lines 394 - 408, The helper hasUserEditableContent currently treats any
userEditOperations as making the panel editable, which causes empty panels for
standalone ops like RETIME_PIECE; change it so it only returns true when
userEditProperties contains real content OR when userEditOperations includes at
least one operation that is actually rendered in the PropertiesPanel.
Concretely, update hasUserEditableContent to filter userEditOperations (e.g.
obj.userEditOperations.some(op => /* op has panel UI */ op.properties ||
op.fields || op.panel || allowedTypes.includes(op.type))) and ignore known
standalone ops such as RETIME_PIECE (see SourceLayerItem.tsx) so only ops that
provide panel fields cause a true result.