design review: timer #1458
design review: timer #1458
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request involves a comprehensive restructuring of the timer-related components and utilities across multiple files in the client application. The changes include relocating the Changes
Possibly related PRs
Suggested Labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cpvalente
force-pushed
the
timer-review
branch
from
3feb59f to
0f50170
Compare
cpvalente
force-pushed
the
timer-review
branch
3 times, most recently
from
3c91d00 to
1d5d063
Compare
cpvalente
force-pushed
the
timer-review
branch
from
1fa2e8b to
1292d54
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
apps/client/src/views/timer/timer.animations.ts (1)
5-18: Consider honoring reduced motion preferences.
A 1-second transition duration may be too lengthy for some users, and you could improve accessibility by conditionally reducing or disabling animations whenprefers-reduced-motionis enabled at the OS level.apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.tsx (1)
17-17: New optional propertyhideOvertimeis clear.
Document it in the prop definition or component usage notes to help others understand its behavior.apps/client/src/views/timer/timer.utils.ts (5)
18-20: Consider trimming whitespace-only messages.
getShowMessageonly checks for an empty string but not whitespace. If whitespace-only messages are to be hidden, consider trimming the text before evaluation.
25-27: Check for any additional playback states.
getIsPlayingreturnstrueforPlayorRoll. If other playback states exist (like pause, or a newly introduced custom state), ensure each is accounted for or intentionally excluded.
46-48: Naming could be more explicit.
getShowClockreturning false forTimerType.Clockcan be confusing at a glance. Consider a more self-descriptive function name, likeshouldHideClockForType.
95-109: Additional conditions for showing modifiers could be consolidated.The logic for
showEndMessage,showFinished,showWarning, andshowDangeris straightforward, but you might unify common conditions, reduce repetition, or incorporate them in a single object with typed states.
149-189: Potential to extract sub-functions for clarity.
getCardDatamanages multiple conditions for current and next events. To improve readability, consider splitting out the “stop” scenario, “active” scenario, and “pending” scenario into smaller helper functions.apps/client/src/views/timer/Timer.tsx (3)
41-52: Validate prop defaults and usage.The
TimerPropsinterface is quite extensive. For better maintainability, consider providing default values (or internal fallbacks) for props such ascustomFieldsandviewSettingsif they can sometimes be undefined. This ensures the component doesn’t break in edge cases.
74-74: Address the TODO regarding size issues.The TODO comment indicates a known issue with displaying minutes. Consider prioritizing a fix or fallback approach to avoid rendering inconsistencies in strict layouts.
90-99: Ensure data completeness for card display.When using
mainSourceandsecondarySourceto determine card data, check that you handle scenarios in whicheventNoworeventNextfields are missing or incomplete. Consider adding a safeguard to avoid rendering partial or null states.apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.scss (1)
3-3: Use caution with fixed height progress bars.Changing
$progress-bar-sizeto1.5remmay affect vertical spacing in some layouts. Ensure it doesn’t overlap other elements on smaller screens or constrained containers.apps/client/src/theme/_viewerDefs.scss (1)
38-38: Check new clamp range for$timer-value-size.Switching from
2rem, 3.5vw, 3.5remto32px, 3.5vw, 50pxtightens the minimum and maximum sizes. Verify that 32px isn’t overly large on small displays or too small on higher-end screens for your typical user base.apps/server/src/user/styles/override.css (1)
50-54: Consider using semantic naming for studio variables.The studio-specific variables could be more semantically named to better reflect their purpose.
Consider renaming:
--- studio-active: #101010; +++ studio-active-background: #101010; --- studio-idle: #cfcfcf; +++ studio-idle-background: #cfcfcf;apps/client/src/views/timer/Timer.scss (1)
124-128: Consider using CSS custom properties for phase colors.The phase-based color system could be more maintainable using CSS custom properties.
- &[data-phase="warning"] { - color: var(--timer-warning-color-override, var(--phase-color)); - } - &[data-phase="danger"] { - color: var(--timer-danger-color-override, var(--phase-color)); - } + @each $phase in (warning, danger) { + &[data-phase="#{$phase}"] { + color: var(--timer-#{$phase}-color-override, var(--phase-color)); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/client/src/AppRouter.tsx(1 hunks)apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.scss(2 hunks)apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.tsx(4 hunks)apps/client/src/common/components/title-card/TitleCard.scss(3 hunks)apps/client/src/common/components/title-card/TitleCard.tsx(1 hunks)apps/client/src/features/operator/status-bar/StatusBar.module.scss(1 hunks)apps/client/src/features/viewers/common/superscript-time/SuperscriptTime.scss(1 hunks)apps/client/src/features/viewers/common/superscript-time/SuperscriptTime.tsx(1 hunks)apps/client/src/features/viewers/common/viewUtils.ts(2 hunks)apps/client/src/features/viewers/timer/Timer.tsx(0 hunks)apps/client/src/features/viewers/timer/timer.options.ts(0 hunks)apps/client/src/theme/_viewerDefs.scss(2 hunks)apps/client/src/views/cuesheet/cuesheet.options.ts(1 hunks)apps/client/src/views/timer/Timer.scss(9 hunks)apps/client/src/views/timer/Timer.tsx(1 hunks)apps/client/src/views/timer/timer.animations.ts(1 hunks)apps/client/src/views/timer/timer.options.ts(1 hunks)apps/client/src/views/timer/timer.utils.ts(1 hunks)apps/server/src/user/styles/override.css(1 hunks)packages/utils/src/validate-events/validateEvent.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/client/src/features/viewers/timer/timer.options.ts
- apps/client/src/features/viewers/timer/Timer.tsx
🔇 Additional comments (43)
apps/client/src/features/operator/status-bar/StatusBar.module.scss (1)
Line range hint
108-111: Verify progress bar height consistency and improve documentation.
- The height of
1remhere differs from the1.5reminMultiPartProgressBar.scss. Please verify if this difference is intentional.- The CSS custom property
--progress-bar-brwould benefit from documentation explaining its purpose.Consider renaming the class to be more semantic, e.g.:
-.progressOverride { +.progress-bar--compact { height: 1rem; --progress-bar-br: 0; }apps/client/src/features/viewers/common/superscript-time/SuperscriptTime.tsx (5)
12-13: Good documentation practice.The comment succinctly indicates the intention behind separating seconds from the main time string, which increases maintainability and clarity.
14-20: Edge-case handling suggestion.While the function correctly handles the standard 8-character input (e.g., "HH:MM:SS"), consider whether you need stricter validation or formatting for unusual cases (e.g., "12:0:9", user typos, or partial times). Currently, inputs that are not exactly 8 characters simply return the original string. If these are truly out-of-scope, adding a comment clarifying expectations would help future maintainers.
22-25: Effective examples in documentation.Showing an example for typical HH:MM behavior and an example including seconds helps users quickly grasp the purpose of the subscript formatting. Great job proactively clarifying usage!
32-32: Validate split usage for advanced formats.Splitting by space to separate a period-like segment (e.g., "AM", or "PM") is straightforward but may have edge cases if users pass more than one space or unexpected text. If multiple tokens are possible, consider adopting a more robust approach (e.g., RegEx) or ensuring usage strictly follows your format requirements.
36-37: Sensible conditional rendering of seconds.Displaying the subscript element only when seconds exist keeps the markup clean and avoids empty spans. This approach is both readable and efficient.
apps/client/src/features/viewers/common/superscript-time/SuperscriptTime.scss (1)
5-8: Consistent styling approach.Defining
.subscriptas a separate class with a suitable font size aligns well with the existing.periodclass. This fosters consistent, modular styling.apps/client/src/views/timer/timer.animations.ts (3)
1-1: Import from 'framer-motion' is appropriate.
No issues here.
3-3: Ensure the relative import path forTitleCardis valid.
Double-check that this path correctly reflects the file structure to avoid runtime errors.
20-20: Exporting a motion-wrapped TitleCard is a neat approach.
This pattern cleanly separates animation specifics from the original component.apps/client/src/common/components/title-card/TitleCard.tsx (1)
8-8: Be mindful of null/undefined titles.
Making the title optional is fine, but ensure the UI gracefully handles an empty or missing title. Consider a fallback to avoid empty text elements.packages/utils/src/validate-events/validateEvent.test.ts (1)
30-33: Appreciate the added test for null timer types.
This strengthens the function’s coverage. You might also consider tests forundefinedor other unexpected input types.apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.tsx (5)
4-4: Usingcxis a good practice for conditional classNames.
Ensures maintainable and clean code, no issues here.
32-32: Consistent destructuring ofhideOvertime.
This maintains clarity in reading the function signature.
40-41: Logic for detecting overtime is straightforward.
ImplementingisOvertimeasnow < 0is concise.
45-50: Neat usage ofcxfor dynamic class handling.
The array-based approach keeps the className logic succinct.
61-62: Conditionally masking danger color for overtime is a smart move.
Cleanly differentiates normal danger mode from overtime mode.apps/client/src/views/cuesheet/cuesheet.options.ts (1)
79-79: Ensure no external dependencies rely on this function export removal.By removing the
exportkeyword fromgetOptionsFromParams, you've effectively made it private to this module. Verify that no other modules rely on this function externally. If so, they will break after this change.apps/client/src/features/viewers/common/viewUtils.ts (4)
10-14: Validate all callers for updated function signature.Adding the optional parameter
timerTypeOverridechanges the function signature. Please ensure all call sites are updated accordingly to avoid runtime errors.
19-20: Fallback logic is sound.Using the nullish coalescing operator here properly respects overrides while preserving the original timer type when not provided. Nicely done.
28-28: Consistency check forviewTimerTypein switch statement.Switching on
viewTimerTypeis straightforward and more explicit. This improves readability by clearly distinguishing between the override scenario and the default scenario.
40-41: Simplified default clause.Falling back to
nullfor unrecognized timer types is a clean approach to handle unexpected input.apps/client/src/views/timer/timer.options.ts (4)
16-21: Confirm omission ofNonetimer type.You excluded
TimerType.NonefromtimerDisplayOptions. If this omission is intentional (e.g., you never want to explicitly set a "none" override), it’s fine; otherwise, consider adding it to ensure consistency of user-selectable options.
23-95: General approach for building timer options is well-structured.The code effectively composes an array of
ViewOptionobjects by combining preset configurations with custom fields. This pattern is clean and maintainable, providing an extensible basis for future timer settings.
110-132: Successful adaptation of search param parsing.This function mirrors the structure of your cuesheet param parsing, ensuring consistency for timer-specific options. Just confirm that all property names align with their definitions in
TimerOptions.
134-141:useTimerOptionsusage highlights a good reactive pattern.Wrapping the param parsing with
useMemoensures the computation is optimized. This approach is efficient for large-scale or repetitive re-renders.apps/client/src/views/timer/timer.utils.ts (4)
32-34: Straightforward time calculation.
getTotalTimesimply sums duration and addedTime, handling null with defaults. This is concise and reliable.
39-41: Progress bar visibility logic is clear.Skipping the progress bar for
NoneandClockmaintains alignment with typical user expectations for these timer types.
114-118: Color override hierarchy.You handle priority for warning and danger states well in
getTimerColour. If no color is provided, consider a fallback to ensure the timer remains visible.
123-144: Validate mismatched external sources.
getSecondaryDisplayhandles 'aux' or 'external' sources. If a developer setssecondarySourceto 'aux' but the message is actually using an 'external' text, ensure the code or documentation clarifies the precedence logic.apps/client/src/views/timer/Timer.tsx (2)
79-89: Verify logic around timer modifiers.The evaluation of modifiers (warning, danger, end message) is helpful for controlling visual states. However, if
time.timerTypeortime.phasebecomes undefined, this could lead to unexpected states. Ensure that calling code or hooks provide valid data and add fallback checks if needed.
191-225: Check animation resource usage.Two separate
<AnimatePresence>blocks are used for showing “Now” and “Next” events. While this is valid, consider consolidating them or verifying that each block doesn’t unintentionally re-render elements in quick succession, potentially impacting performance.apps/client/src/common/components/title-card/TitleCard.scss (4)
3-5: Confirm default size behavior.
$title-primary-sizeand$title-secondary-sizeuse clamp with minimums of1.5remand1remrespectively. For smaller devices or unique layouts, verify these sizes aren’t too large or overlapping.
21-21: Good substitution for responsive sizing.Switching to
$title-primary-sizein.title-card__titleis an improvement for maintainability and readability of text across various screen sizes.
27-27: Align secondary text styling.Using
$title-secondary-sizeensures consistency with the primary title. This helps preserve a coherent hierarchy in the title card design.
36-36: Maintain consistent label styling.Adopting
$timer-label-sizefor.title-card__labelprovides uniformity across components, especially when used in tandem with timer-based layouts.apps/client/src/common/components/multi-part-progress-bar/MultiPartProgressBar.scss (1)
56-58: Approved: Overtime color logic.Your new
.multiprogress-bar__bg-danger--overtimeclass effectively differentiates overtime from regular danger states. This clarifies the UI for edge cases where the timer has exceeded its intended range.apps/client/src/theme/_viewerDefs.scss (1)
24-28: Validate new view-element sizing variables.The newly introduced
$view-element-gap,$view-outer-padding, and$view-card-paddingdefine padding using a combination ofmin(),clamp(), and fixed values. Ensure these values display properly across an array of screen resolutions, including very small or unusually large devices.apps/server/src/user/styles/override.css (1)
25-27: LGTM! Timer color overrides enhance customization.The new timer color override variables for warning, danger, and overtime states provide better customization options for different timer phases.
apps/client/src/views/timer/Timer.scss (3)
14-18: LGTM! Improved layout structure.The switch from grid to flexbox with standardized spacing variables improves maintainability and responsiveness.
201-202: LGTM! Responsive logo positioning.The use of min() function for logo positioning ensures proper scaling across different viewport sizes.
138-145: Verify the impact of line-height changes.The line height changes and new spacing values might affect text rendering.
✅ Verification successful
Line height change is safe and localized ✅
The line-height change to 1em in Timer.scss only affects the Timer component's secondary text display. This is a standard value that ensures consistent text rendering and proper vertical alignment. Other components using "secondary" classes have their own independent styling modules and are not affected by this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components using the secondary class rg -A 5 "className.*secondary" --glob "**/*.{tsx,jsx}"Length of output: 3566
Script:
#!/bin/bash # Search for all SCSS/CSS files with line-height or secondary class definitions rg -A 3 "line-height|\.secondary" --glob "**/*.{scss,css}"Length of output: 13109
apps/client/src/AppRouter.tsx (1)
25-25: Verify the new Timer component import path.Ensure that all references to the Timer component have been updated consistently across the codebase.
✅ Verification successful
Timer component import path is correctly updated
The new import path './views/timer/Timer' in AppRouter.tsx is correct and consistent with the actual file location. No outdated references were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old timer path rg -l "features/viewers/timer/Timer" --glob "**/*.{tsx,ts,jsx,js}"Length of output: 66
Script:
#!/bin/bash # Check if the new Timer component exists fd Timer.tsx -t f # Search for any Timer-related imports rg "import.*Timer" --glob "**/*.{tsx,ts,jsx,js}" -l # Search for files containing "timer" case-insensitive fd -i timer -t f -e tsx -e ts -e jsx -e jsLength of output: 5838
cpvalente
force-pushed
the
timer-review
branch
from
1292d54 to
a4d446e
Compare
Starting the design review with the timer and associated components. This is divided in two parts
Style and code tweaks
Behaviour and structure changes