-
-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: event finder #1278
feat: event finder #1278
Conversation
c1c8577
to
7a4efcf
Compare
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 introduces significant enhancements to the keyboard shortcut handling and the Finder feature within the Editor component. The Changes
Possibly related PRs
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 (
|
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
🧹 Outside diff range and nitpick comments (10)
apps/client/src/theme/ontimeModal.ts (1)
15-15
: LGTM! Consider using a CSS variable for consistency.The addition of a border to the dialog is a good improvement for visual clarity. It helps define the modal's boundaries, especially against the dark background.
For consistency with modern CSS practices and to improve maintainability, consider using a CSS variable for the border color. This would allow for easier theming and updates in the future.
Here's a suggestion for using a CSS variable:
- border: '1px solid #2d2d2d', // $gray-1100 + border: '1px solid var(--gray-1100)',Make sure to define the
--gray-1100
variable in your global CSS with the value#2d2d2d
.apps/client/src/features/editors/finder/Finder.module.scss (1)
1-64
: Overall well-structured SCSS module with room for minor improvementsThe Finder.module.scss file is well-implemented, utilizing modern CSS features and maintaining consistency in styling approaches. Here's a summary of the strengths and areas for improvement:
Strengths:
- Consistent use of color variables for maintainability.
- Appropriate use of CSS Grid and Flexbox for layouts.
- Clear structure with separate styles for different components.
Suggestions for improvement:
- Consider using a placeholder selector for shared styles (as mentioned in a previous comment).
- Add comments to document the purpose of each major section or class.
To improve documentation, consider adding comments like this:
// Styles for list entries, including empty state and error messages .entry, .empty, .error { // ... } // Grid layout for the main data display .data { // ... } // Styles for the footer section .footer { // ... } // Emphasis styles for highlighted text .em { // ... }These comments will help other developers understand the purpose of each section quickly.
apps/client/src/features/editors/Editor.tsx (3)
25-25
: LGTM: Good state management for FinderThe use of
useDisclosure
hook for managing the Finder's open/close state is a good practice. It provides a clean and reusable way to handle the visibility of the Finder component.Consider extracting the Finder-related logic into a custom hook (e.g.,
useFinder
) if it grows in complexity. This would improve the separation of concerns and make the Editor component more focused. For example:const useFinder = () => { const { isOpen, onToggle, onClose } = useDisclosure(); // Add any additional Finder-related logic here return { isFinderOpen: isOpen, onFinderToggle: onToggle, onFinderClose: onClose }; }; // In the Editor component const { isFinderOpen, onFinderToggle, onFinderClose } = useFinder();
38-50
: LGTM: Improved keyboard shortcut handlingThe refactoring of the
toggleSettings
function and the introduction ofuseHotkeys
for keyboard shortcut management is a significant improvement. It makes the code more declarative and easier to maintain.Consider extracting the hotkey definitions into a separate constant or configuration object. This would make it easier to manage and potentially reuse these shortcuts across the application. For example:
const EDITOR_HOTKEYS = [ ['mod + ,', toggleSettings], ['mod + f', onFinderToggle], ['Escape', onFinderClose], ] as const; // In the component useHotkeys(EDITOR_HOTKEYS);This approach would also make it easier to add documentation for each shortcut if needed in the future.
54-54
: LGTM: Finder component integrated correctlyThe Finder component has been properly integrated into the Editor's JSX. It's correctly using the state and handlers defined earlier, which ensures consistent behavior.
Consider adding a data-testid attribute to the Finder component for easier testing. This would allow for more robust end-to-end or integration tests. For example:
<Finder isOpen={isFinderOpen} onClose={onFinderClose} data-testid="event-finder" />This addition would make it easier to select and interact with the Finder component in automated tests.
apps/client/src/features/rundown/event-editor/EventEditorEmpty.tsx (1)
17-33
: LGTM! Consider adding aria-labels for accessibility.The new shortcuts for "Open Finder" and "Open Settings" are well-integrated and maintain consistency with the existing structure. The added spacer row improves visual separation and readability.
To enhance accessibility, consider adding aria-labels to the
<Kbd>
components. For example:- <Kbd>{deviceMod}</Kbd> + <Kbd aria-label="Command key">{deviceMod}</Kbd>This change would improve screen reader support for users relying on assistive technologies.
apps/client/src/features/editors/finder/Finder.tsx (1)
70-71
: Fix spacing issues in the footer textDue to line breaks in JSX, there might be missing spaces in the rendered footer text between words, which can cause the text to appear concatenated.
Adjust the code to ensure spaces are rendered correctly:
- Use the keywords <span className={style.em}>cue</span>, <span className={style.em}>index</span> or - <span className={style.em}>title</span> to filter search + Use the keywords <span className={style.em}>cue</span>, <span className={style.em}>index</span> or{' '} + <span className={style.em}>title</span> to filter searchapps/client/src/features/editors/finder/useFinder.tsx (3)
39-41
: Provide an error message when the search index is out of boundsWhen the
searchIndex
exceeds the length of the data, returning an error message improves user feedback by informing them that the index is out of bounds.Suggested change:
if (searchIndex > data.length) { - return { results: [], error: null }; + return { results: [], error: 'Index out of bounds' }; }
100-100
: Update comment to reflect that both events and blocks are searchedThe comment for
searchByTitle
mentions onlyOntimeEvents
, but the function also searchesOntimeBlocks
. Updating the comment ensures it accurately describes the function's behavior.Suggested change:
- /** Returns maxResults of OntimeEvents that match the title field*/ + /** Returns maxResults of OntimeEvents and OntimeBlocks that match the title field */
108-140
: Rename variableevent
toitem
for clarityIn the
searchByTitle
function, the variableevent
represents both events and blocks. Renaming it toitem
improves readability and reduces potential confusion.Apply this diff:
for (let i = 0; i < data.length; i++) { if (remaining <= 0) { break; } - const event = data[i]; + const item = data[i]; - if (isOntimeEvent(event)) { - if (event.title.toLowerCase().includes(searchString)) { + if (isOntimeEvent(item)) { + if (item.title.toLowerCase().includes(searchString)) { remaining--; results.push({ type: SupportedEvent.Event, id: item.id, index: i, eventIndex, title: item.title, cue: item.cue, colour: item.colour, } satisfies FilterableEvent); } eventIndex++; } - if (isOntimeBlock(event)) { - if (event.title.toLowerCase().includes(searchString)) { + if (isOntimeBlock(item)) { + if (item.title.toLowerCase().includes(searchString)) { remaining--; results.push({ type: SupportedEvent.Block, id: item.id, index: i, title: item.title, } satisfies FilterableBlock); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
apps/client/package.json
is excluded by none and included by nonepnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
and included by none
📒 Files selected for processing (6)
- apps/client/src/features/editors/Editor.tsx (3 hunks)
- apps/client/src/features/editors/finder/Finder.module.scss (1 hunks)
- apps/client/src/features/editors/finder/Finder.tsx (1 hunks)
- apps/client/src/features/editors/finder/useFinder.tsx (1 hunks)
- apps/client/src/features/rundown/event-editor/EventEditorEmpty.tsx (1 hunks)
- apps/client/src/theme/ontimeModal.ts (1 hunks)
🧰 Additional context used
🪛 Biome
apps/client/src/features/editors/finder/useFinder.tsx
[error] 35-35: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (6)
apps/client/src/features/editors/finder/Finder.module.scss (3)
25-54
: Well-structured grid layout and nested stylesThe
.data
class and its nested elements are well-implemented using CSS Grid. The layout is flexible and the styles for each element are appropriately defined.Good practices observed:
- Use of CSS Grid for complex layouts.
- Clear naming of grid areas for better readability.
- Use of
calc()
for font sizes, ensuring consistency with the base font size.- Appropriate use of color variables for maintainability.
56-59
: Footer styles are appropriateThe
.footer
class styles are simple and consistent with the overall styling approach. The use ofcalc()
for the font size maintains consistency with other elements.
61-64
: Emphasis styles are appropriateThe
.em
class styles are concise and use color variables consistently. The margin settings provide appropriate spacing for emphasized elements.apps/client/src/features/editors/Editor.tsx (3)
3-3
: LGTM: Good choice for keyboard shortcut managementThe addition of
useHotkeys
from@mantine/hooks
is a positive change. This hook provides a more declarative and React-friendly way to manage keyboard shortcuts, which can lead to cleaner and more maintainable code.
Line range hint
1-91
: Overall assessment: Significant improvements and new feature integrationThe changes to the Editor component represent a substantial improvement in code quality and functionality:
- The introduction of
useHotkeys
for keyboard shortcut management simplifies the code and makes it more maintainable.- The integration of the new Finder component aligns with the PR objective of providing an event search interface.
- The use of
useDisclosure
for state management of the Finder component is a clean and effective approach.These changes enhance the user experience by providing better keyboard navigation and search capabilities. The code is now more React-idiomatic and easier to maintain.
To further improve the implementation, consider the suggestions made in the previous comments, such as:
- Extracting Finder-related logic into a custom hook
- Defining hotkeys in a separate constant
- Adding data-testid attributes for testing
Great work on this feature addition and code improvement!
14-15
: LGTM: New Finder component importedThe import of the new
Finder
component aligns with the PR objective of providing an interface for searching events in the rundown. This is a good addition to enhance the functionality of the Editor.To ensure the Finder component is properly implemented, let's verify its contents:
✅ Verification successful
Verified: Finder component implementation confirmed
The
Finder
component includes a search interface and the necessary logic for searching events, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the Finder component # Expected result: The Finder component should have a search interface and related logic # Test: Check the contents of the Finder component fd -e tsx -e ts Finder.tsx | xargs catLength of output: 7922
7a4efcf
to
2ae3c38
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.
this is a bit forced, but if we could find a way to accommodate it, that would be good
I like having it function on keyboard only but maybe it would be ok to make the filter buttons instead off keywords only?
to get around some of these issues, we could then add it later when we have a good solution
This is a good point. Maybe we should informe a tagging system like
I forgot about mouse, but I guess it is ok as is? I think adding interactive fields there compromises us to maintain this going forwards, so I believe it is best we do this at a latter point. |
2ae3c38
to
41dd866
Compare
41dd866
to
6bf4d15
Compare
feature to pair up with reporter.
It provides an interface for searching an event in the rundown
I guess this could be easily extendable to have a list of settings shortcuts like network view settings
With the current setup this would mean typing
settings network
orsettings view
.Perhaps we would prefer having a character to signify our tags? like
>cue
or>settings
?We will be a bit stuck with whatever we decide now