Conversation
|
✅ Codex review complete; no issues found View full reviewAg 16449/marker drawing modesPR: #6100 SummaryAdjusts marker drawing mode handling to account for opacity and user-provided highlight settings, and wires options graph user-option checks into series logic. Updates a few tests and snapshots to cover the new highlight drawing behaviour. FindingsP0: 0 | P1: 0 | P2: 0 | P3: 0 No issues found. VerdictAssessment: correct I did not find any correctness, performance, or maintainability issues introduced by the changes in the diff. Required Actions: None - ready to merge |
|
Snapshots automatically updated, please review before merge: |
|
|
||
| protected override shouldIgnoreDefaultMarkerFillOpacity(): boolean { | ||
| const seriesPath = ['series', `${this.declarationOrder}`]; | ||
| return !this.ctx.optionsGraphService.hasUserOption([...seriesPath, 'fillOpacity']); |
There was a problem hiding this comment.
Can this be determined through the graph instead? For example drawingMode: { $isUserOption: ['./fillOpacity', true, false] } as an undocumented option.
There was a problem hiding this comment.
actually that might be a really good idea
| hasUserOption(path: Array<string>) { | ||
| return this.hasUserOptionCallback?.(path) ?? false; |
There was a problem hiding this comment.
We should not expose the options graph without requiring calls to be processed through the graph itself. Otherwise this will lead to bugs where the order of node dependency resolution is not correct.
| hasUserOption(path: Array<string>) { | ||
| const hasUserOptionSimple = hasPathSafe(this.userOptions, path); | ||
| if (hasUserOptionSimple) return true; | ||
| if (!this.root) return false; |
There was a problem hiding this comment.
The reason this is needed is because the graph is deleted after it is resolved. This a reason why we shouldn't make direct calls into the graph externally, but instead should utilise graph operations, i.e. $isUserOption.
So, we shouldn't need this line.
manapeirov
force-pushed
the
AG-16449/marker-drawing-modes
branch
2 times, most recently
from
297d76b to
d7a9b3c
Compare
|
Snapshots automatically updated, please review before merge: |
|
Snapshots automatically updated, please review before merge: |
manapeirov
force-pushed
the
AG-16449/marker-drawing-modes
branch
from
809847f to
6993588
Compare
|
Snapshots automatically updated, please review before merge: |
|
Snapshots automatically updated, please review before merge: |
|
Snapshots automatically updated, please review before merge: |
|
Snapshots automatically updated, please review before merge: |
|
Snapshots automatically updated, please review before merge: |
…rawing-modes Gha/snapshots ag 16449/marker drawing modes
3 participants
PR for https://ag-grid.atlassian.net/browse/AG-16449