feat: improve UX in properties editor#1620
feat: improve UX in properties editor#1620anteeek wants to merge 2 commits intoSofie-Automation:mainfrom
Conversation
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<br/>(double-click)
participant EventBus as RundownViewEventBus
participant RundownView
participant Panels as Notification/Properties<br/>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 effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
|
Note: this PR was created with wrong target first in #1620 62 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Saftret
added
the
Contribution from BBC
|
@nytamin Images are now added to the description |
Julusian
force-pushed
the
feat/SOFIE-284
branch
from
b3ebcf3 to
d43cf78
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/UserEditingDefinitions` 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.
ℹ️ 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.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '230,250p'Repository: Sofie-Automation/sofie-core
Length of output: 564
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '220,260p'Repository: Sofie-Automation/sofie-core
Length of output: 985
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '200,270p'Repository: Sofie-Automation/sofie-core
Length of output: 1897
🏁 Script executed:
rg -n "propertiespanel-pop-up__button" packages/webui/src/client/styles/propertiesPanel.scssRepository: Sofie-Automation/sofie-core
Length of output: 331
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '110,130p'Repository: Sofie-Automation/sofie-core
Length of output: 664
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '95,125p'Repository: Sofie-Automation/sofie-core
Length of output: 894
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '170,240p'Repository: Sofie-Automation/sofie-core
Length of output: 1826
🏁 Script executed:
cat -n packages/webui/src/client/styles/propertiesPanel.scss | sed -n '1,50p'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
.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;📝 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.
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
right: 0;
-
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.
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
private onCloseNotifications = () => {
+ 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.
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
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()
}📝 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/SegmentTimeline.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.
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
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
)
}📝 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/PropertiesPanel.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.
4 participants
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
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