feat: implement VerticalResource view with layout and styling updates#344
Open
ansulagrawal wants to merge 5 commits into
Open
feat: implement VerticalResource view with layout and styling updates#344ansulagrawal wants to merge 5 commits into
ansulagrawal wants to merge 5 commits into
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a VerticalResource view mode and routes SchedulerData, header/body rendering, event placement, layout/CSS, and example routing to render resources vertically by time. ChangesVertical Resource View Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/examples/pages/Drag-And-Drop/class-based.jsx (1)
58-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Biome-blocking prop indentation in
<Scheduler>CI is currently failing because the JSX props under
<Scheduler>are not indented as Biome expects (see Line 62-Line 78). This blocks merge.Suggested formatting fix
<div ref={this.schedulerContainerRef} style={{ width: '100%', overflow: 'hidden' }}> <Scheduler CustomResourceHeader={() => <div>Custom Header</div>} schedulerData={viewModel} parentRef={this.schedulerContainerRef} - prevClick={this.prevClick} - nextClick={this.nextClick} - onSelectDate={this.onSelectDate} - onViewChange={this.onViewChange} - eventItemClick={this.eventClicked} - viewEventClick={this.ops1} - viewEventText="Ops 1" - viewEvent2Text="Ops 2" - viewEvent2Click={this.ops2} - updateEventStart={this.updateEventStart} - updateEventEnd={this.updateEventEnd} - moveEvent={this.moveEvent} - movingEvent={this.movingEvent} - newEvent={this.newEvent} - subtitleGetter={this.subtitleGetter} - dndSources={[taskDndSource, resourceDndSource]} - toggleExpandFunc={this.toggleExpandFunc} - /> + prevClick={this.prevClick} + nextClick={this.nextClick} + onSelectDate={this.onSelectDate} + onViewChange={this.onViewChange} + eventItemClick={this.eventClicked} + viewEventClick={this.ops1} + viewEventText="Ops 1" + viewEvent2Text="Ops 2" + viewEvent2Click={this.ops2} + updateEventStart={this.updateEventStart} + updateEventEnd={this.updateEventEnd} + moveEvent={this.moveEvent} + movingEvent={this.movingEvent} + newEvent={this.newEvent} + subtitleGetter={this.subtitleGetter} + dndSources={[taskDndSource, resourceDndSource]} + toggleExpandFunc={this.toggleExpandFunc} + /> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/examples/pages/Drag-And-Drop/class-based.jsx` around lines 58 - 79, The JSX props on the <Scheduler> component are misindented and failing Biome; reformat the <Scheduler> element so each prop (e.g., schedulerData, parentRef, prevClick, nextClick, onSelectDate, onViewChange, eventItemClick, viewEventClick, viewEvent2Click, updateEventStart, updateEventEnd, moveEvent, movingEvent, newEvent, subtitleGetter, dndSources, toggleExpandFunc) is aligned consistently (one prop per line indented to the same level beneath the opening <Scheduler> tag) and ensure the closing "/>" is on its own line; update the prop indentation to match the existing first props (CustomResourceHeader and schedulerData) so Biome formatting passes.src/components/EventItem.jsx (1)
528-534:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse dynamic line-height when dynamic
heightis provided.Line 533 always uses
config.eventItemHeight, which can visually misalign text when Line 528 applies a different runtimeheight.💡 Proposed fix
<span style={{ marginLeft: '10px', - lineHeight: `${config.eventItemHeight}px`, + lineHeight: `${height || config.eventItemHeight}px`, }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/EventItem.jsx` around lines 528 - 534, The span's inline style uses a fixed lineHeight of config.eventItemHeight which desynchronizes when the container uses a dynamic height prop; update the EventItem component to compute the actual height (e.g., use the existing height prop fallback pattern: actualHeight = height || config.eventItemHeight) and then set lineHeight to `${actualHeight}px` (replace the direct use of config.eventItemHeight) so the text vertical alignment follows the runtime height.src/components/SchedulerData.js (1)
287-321:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix incorrect date-window initialization when entering VerticalResource view.
On Line 287, the
this.viewType < viewTypebranch never handlesViewType.VerticalResource. Switching from Week/Month/Year into Vertical can keep a multi-day range, producing oversized hourly slot grids.💡 Proposed fix
- if (this.viewType < viewType) { - if (viewType === ViewType.Week) { + if (this.viewType < viewType) { + if (viewType === ViewType.Day || viewType === ViewType.VerticalResource) { + this.startDate = this.localeDayjs(new Date(date)).startOf('day'); + this.endDate = this.startDate; + this.cellUnit = CellUnit.Hour; + } else if (viewType === ViewType.Week) { this.startDate = this.localeDayjs(new Date(date)).startOf('isoWeek'); this.endDate = this.localeDayjs(new Date(this.startDate)).endOf('isoWeek'); } else if (viewType === ViewType.Month) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SchedulerData.js` around lines 287 - 321, The code path handling upgrades of viewType (the if block guarded by "if (this.viewType < viewType)") never handles ViewType.VerticalResource, so switching into VerticalResource from a larger range keeps multi-day startDate/endDate and yields oversized hourly grids; in the same branch where Week/Month/Quarter/Year are set, add a case for viewType === ViewType.VerticalResource (or combine it with ViewType.Day) to set this.startDate = this.localeDayjs(new Date(date)).startOf('day'), this.endDate = this.startDate, and this.cellUnit = CellUnit.Hour so the date window is a single day when entering the VerticalResource view.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/HeaderView.jsx`:
- Around line 63-67: Guard the week-number header rendering so it only runs when
week numbers are valid and vertical-resource mode is not enabled: in the header
rendering blocks that reference group.week and group.year (the <th
key={`week-${group.year}-${group.week}`} .../> block and the similar block
around lines 127-136), wrap the render with a check for showWeekNumber &&
!<vertical-resource-mode-flag> (or at minimum ensure group.week is
defined/non-null) so we skip producing week-number groups when vertical-resource
mode is active; update both occurrences to use the same guard to prevent invalid
week values and keys.
In `@src/components/SchedulerData.js`:
- Around line 930-935: The vertical-resource branch sets header.time to a
non-date identifier (this.headers = this.resources.map(... time: r.id ...)), but
later code in build/iterate over headers (where header.time is parsed as a Date
and header.start/header.end are computed) expects a Date/time value; update the
vertical case in isVerticalResourceView() to assign a proper date/time value
(e.g., the current view date or the existing visible date value used elsewhere)
to this.headers' time field and ensure nonWorkingTime stays correct so
subsequent parsing of header.time produces valid header.start/header.end ranges;
locate and change the assignment in the block that builds headers for vertical
resource view (the map assigning time: r.id) and make it consistent with the
header parsing logic.
- Around line 424-427: The responsive height calculation is subtracting the
week-number row twice: totalHeaderHeight currently adds week-number height but
headerHeight (e.g., schedulerHeaderHeight in the parent observer) already
includes that contribution. Modify the computation in SchedulerData by removing
the week-number addition from totalHeaderHeight (or simply use headerHeight
where appropriate) so baseHeight is computed as this.documentHeight -
headerHeight - (this.config.underneathHeight || 0) and avoid subtracting
showWeekNumber ? weekNumberRowHeight twice; update references to
totalHeaderHeight, baseHeight, this.config.tableHeaderHeight,
this.config.showWeekNumber, this.config.weekNumberRowHeight and headerHeight
accordingly.
- Around line 132-136: addResource currently updates resources and calls
_createRenderData but doesn't rebuild headers, causing missing columns in
vertical mode; modify addResource to mirror setResources by validating and
updating the resources collection and then calling this._createHeaders() before
this._createRenderData() (so locate addResource and insert a call to
_createHeaders() after the resources update and before _createRenderData(),
ensuring headers are regenerated when operating in vertical mode).
In `@src/examples/components/TaskItem.jsx`:
- Around line 23-37: The inline visual styles in the TaskItem component (the
style prop currently applied in TaskItem.jsx) should be moved into the
.task-item-hover CSS class to avoid specificity and !important usage; remove the
style prop from the TaskItem JSX, create/extend the .task-item-hover rule in
your stylesheet to include padding, margin, background-color, border,
border-radius, color, font-weight, font-size, list-style, cursor, box-shadow and
transition, and also add the :hover state there for the hover effects so base
and hover styles are consolidated under the .task-item-hover selector.
In `@src/examples/css/style.css`:
- Around line 162-165: Remove the use of !important from the
.task-item-hover:hover rules (background-color, border-color, box-shadow) to
satisfy the noImportantStyles Biome rule and CI, and fix the box-shadow rgba
formatting to include spaces after commas (use "rgba(0, 0, 0, 0.1)" style).
Update the CSS block for the .task-item-hover:hover selector to remove
"!important" on each property and correct the rgba argument spacing so
formatting checks pass.
In `@src/examples/pages/VerticalView/index.jsx`:
- Around line 28-50: The handlers prevClick, nextClick, onSelectDate and
onViewChange currently mutate the schedulerData in place (via
schedulerData.prev()/next()/setDate()/setViewType()) then call setViewModel with
the same object reference, so React may not re-render; fix by passing a new
reference to state after mutation (e.g., clone schedulerData or create a shallow
copy) when calling setViewModel so React sees a changed value — update each
handler (prevClick, nextClick, onSelectDate, onViewChange) to call setViewModel
with a newly created object (or use an existing clone() on schedulerData if
available) instead of the original schedulerData reference.
---
Outside diff comments:
In `@src/components/EventItem.jsx`:
- Around line 528-534: The span's inline style uses a fixed lineHeight of
config.eventItemHeight which desynchronizes when the container uses a dynamic
height prop; update the EventItem component to compute the actual height (e.g.,
use the existing height prop fallback pattern: actualHeight = height ||
config.eventItemHeight) and then set lineHeight to `${actualHeight}px` (replace
the direct use of config.eventItemHeight) so the text vertical alignment follows
the runtime height.
In `@src/components/SchedulerData.js`:
- Around line 287-321: The code path handling upgrades of viewType (the if block
guarded by "if (this.viewType < viewType)") never handles
ViewType.VerticalResource, so switching into VerticalResource from a larger
range keeps multi-day startDate/endDate and yields oversized hourly grids; in
the same branch where Week/Month/Quarter/Year are set, add a case for viewType
=== ViewType.VerticalResource (or combine it with ViewType.Day) to set
this.startDate = this.localeDayjs(new Date(date)).startOf('day'), this.endDate =
this.startDate, and this.cellUnit = CellUnit.Hour so the date window is a single
day when entering the VerticalResource view.
In `@src/examples/pages/Drag-And-Drop/class-based.jsx`:
- Around line 58-79: The JSX props on the <Scheduler> component are misindented
and failing Biome; reformat the <Scheduler> element so each prop (e.g.,
schedulerData, parentRef, prevClick, nextClick, onSelectDate, onViewChange,
eventItemClick, viewEventClick, viewEvent2Click, updateEventStart,
updateEventEnd, moveEvent, movingEvent, newEvent, subtitleGetter, dndSources,
toggleExpandFunc) is aligned consistently (one prop per line indented to the
same level beneath the opening <Scheduler> tag) and ensure the closing "/>" is
on its own line; update the prop indentation to match the existing first props
(CustomResourceHeader and schedulerData) so Biome formatting passes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6da65b35-dac3-40df-8f82-91dcc7740702
📒 Files selected for processing (16)
src/components/BodyView.jsxsrc/components/EventItem.jsxsrc/components/HeaderView.jsxsrc/components/ResourceEvents.jsxsrc/components/ResourceView.jsxsrc/components/SchedulerData.jssrc/components/index.jsxsrc/config/default.jssrc/config/scheduler.jssrc/css/style.csssrc/examples/components/Slider.jsxsrc/examples/components/TaskItem.jsxsrc/examples/css/style.csssrc/examples/pages/Drag-And-Drop/class-based.jsxsrc/examples/pages/VerticalView/index.jsxsrc/examples/router.jsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 5 file(s) based on 6 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 5 file(s) based on 6 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/SchedulerData.js (1)
132-193:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Duplicate
addResourcemethod definitions — the vertical-mode header rebuild is silently lost.
addResourceis declared twice in this class (lines 132-142 and lines 186-193). In JavaScript class bodies, the second definition wins, so the version that callsthis._createHeaders()forisVerticalResourceView()is overridden by the later one that does not. As a result, adding a resource in vertical mode will not regenerate headers, leaving the new resource column missing — defeating the fix from the prior review. Biome is also failing the build on this duplicate.🔧 Proposed fix: remove the second declaration
getMinuteStepsInHour() { return 60 / this.config.minuteStep; } - addResource(resource) { - const existedResources = this.resources.filter(x => x.id === resource.id); - if (existedResources.length === 0) { - this.resources.push(resource); - this._createRenderData(); - this.bumpVersion(); - } - } - addEventGroup(eventGroup) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SchedulerData.js` around lines 132 - 193, There are two duplicate class methods named addResource; the second definition overrides the first and omits the vertical-mode header regeneration. Remove the duplicate addResource (the later one that only calls this._createRenderData() and this.bumpVersion()) so the single addResource implementation used is the one that checks this.isVerticalResourceView() and calls this._createHeaders(), then this._createRenderData() and this.bumpVersion() to restore correct behavior.src/components/HeaderView.jsx (1)
172-183: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDependency
schedulerData.isVerticalResourceViewis a stable method reference, not a value.This dep is the unbound prototype method — it never changes across renders, so it adds no reactivity. The memo already recomputes when
headerschange (andheadersis rebuilt on view-type changes), so today it works incidentally. To make the intent explicit, depend on the call result or onschedulerDataitself.🔧 Suggested change
- schedulerData.isVerticalResourceView, + schedulerData,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/HeaderView.jsx` around lines 172 - 183, The dependency list currently includes the unbound method reference schedulerData.isVerticalResourceView which is stable and doesn't trigger recomputation; replace that entry with the actual boolean result (call schedulerData.isVerticalResourceView()) or depend on schedulerData itself so the memo (the useMemo in HeaderView.jsx surrounding the array of deps including cellUnit, headers, minuteStepsInHour, cellWidth, config, localeDayjs, createCellStyle, getCellFormat, renderCellContent) will re-run when the view orientation changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/SchedulerData.js`:
- Around line 993-996: The code currently assigns id: header.id and style:
header.style when building header items but header.id only exists in vertical
mode, which can produce undefined ids for non-vertical timeSlots; update the
logic in the header construction (where headers/timeSlots are mapped into header
items) to always produce a stable id and style — either by calling/adding an id
generator (e.g., use the time string or index) for header.id when missing or by
defaulting style to header.style || {} — or alternatively guard the assignment
here so id is set to a deterministic fallback; ensure this change is coordinated
with _createHeaders and will keep _createRenderData behavior intact.
- Line 427: The long single-line declaration of baseHeight is failing the
formatter; refactor the baseHeight initialization in SchedulerData.js by
breaking the expression across multiple lines or introducing a small
intermediate const (e.g. const underneathHeight = this.config.underneathHeight
|| 0) and then compute const baseHeight = this.documentHeight - headerHeight -
this.config.tableHeaderHeight - underneathHeight so the expression reflows and
respects line-length rules.
- Around line 1350-1363: The overlap check in SchedulerData.js uses non-existent
ts.start/ts.end on timeSlots (built in _createHeaders as {time, nonWorkingTime,
style}), so remove the dead branches and consistently compute slot start/end
from ts.time and this.config.minuteStep (using
this.localeDayjs(tsStart).add(...)) or alternatively populate start and end when
building timeSlots in _createHeaders; update the loop that references timeSlots
(the this.timeSlots.forEach block) to rely solely on ts.time-derived start/end
or ensure _createHeaders creates start/end fields so the existing branch is
meaningful.
In `@src/examples/components/TaskItem.jsx`:
- Around line 21-26: Move the <li> props onto a single opening tag line so Biome
formatting passes: update the TaskItem.jsx JSX for the <li> that currently uses
ref={dragRef} and className="task-item-hover" (rendering {task.name}) to have
both props on the same line of the <li> start tag instead of each on separate
lines; preserve the dragRef and className values and the child {task.name}
content and ensure no other whitespace/line breaks are introduced in that
opening tag.
In `@src/examples/pages/VerticalView/index.jsx`:
- Around line 28-50: The handlers (prevClick, nextClick, onSelectDate,
onViewChange) currently call setViewModel({ ...schedulerData }), which strips
the SchedulerData prototype and loses methods like
prev/next/setDate/setViewType; replace this with one of two fixes: (A) keep the
original SchedulerData instance and trigger a re-render by updating state with a
counter (e.g., increment a renderCounter via setState) instead of replacing
schedulerData, or (B) create a shallow copy that preserves the prototype (e.g.,
Object.assign(Object.create(Object.getPrototypeOf(schedulerData)),
schedulerData)) and pass that to setViewModel so SchedulerData methods remain
available; update all four handlers (prevClick, nextClick, onSelectDate,
onViewChange) to use the chosen approach.
---
Outside diff comments:
In `@src/components/HeaderView.jsx`:
- Around line 172-183: The dependency list currently includes the unbound method
reference schedulerData.isVerticalResourceView which is stable and doesn't
trigger recomputation; replace that entry with the actual boolean result (call
schedulerData.isVerticalResourceView()) or depend on schedulerData itself so the
memo (the useMemo in HeaderView.jsx surrounding the array of deps including
cellUnit, headers, minuteStepsInHour, cellWidth, config, localeDayjs,
createCellStyle, getCellFormat, renderCellContent) will re-run when the view
orientation changes.
In `@src/components/SchedulerData.js`:
- Around line 132-193: There are two duplicate class methods named addResource;
the second definition overrides the first and omits the vertical-mode header
regeneration. Remove the duplicate addResource (the later one that only calls
this._createRenderData() and this.bumpVersion()) so the single addResource
implementation used is the one that checks this.isVerticalResourceView() and
calls this._createHeaders(), then this._createRenderData() and
this.bumpVersion() to restore correct behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49efcf49-f954-4473-bc4f-3d78ce51ddd7
📒 Files selected for processing (5)
src/components/HeaderView.jsxsrc/components/SchedulerData.jssrc/examples/components/TaskItem.jsxsrc/examples/css/style.csssrc/examples/pages/VerticalView/index.jsx
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 5 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Contributor
|
Note Docstrings generation - SUCCESS |
Fixed 3 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Docstrings generation was requested by @ansulagrawal. The following files were modified: * `src/components/BodyView.jsx` * `src/components/index.jsx` * `src/examples/components/Slider.jsx` * `src/examples/components/TaskItem.jsx` These files were kept as they were: * `src/components/HeaderView.jsx` * `src/components/ResourceView.jsx` These file types are not supported: * `src/css/style.css` * `src/examples/css/style.css`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Todo (Pending Task):
Summary by CodeRabbit
New Features
Improvements