feat(draw): add fillStyle support for shapes#1137
Conversation
|
@Kripu77 is attempting to deploy a commit to the tethys Team on Vercel. A member of the Team first needs to authorize it. |
|
@pubuzhixing8 could you review this please? |
I would review it soon. |
|
@Kripu77 Could you add some functional examples for this feature, such as the mark of below image: |
…lict # Conflicts: # packages/draw/src/engines/flowchart/delay.ts # packages/draw/src/engines/flowchart/predefined-process.ts # packages/draw/src/engines/uml/activity-class.ts # packages/draw/src/engines/uml/container.ts # packages/draw/src/engines/uml/deletion.ts
|
Hi @nightt5879, I improved this pr and added the feature entry on example board, it's now ready for review. |
nightt5879
left a comment
nightt5879
left a comment
There was a problem hiding this comment.
Thanks for adding fillStyle support and the demo entry. I tested the PR locally:
npm run buildpassednpm run build:demopassed, with existing Angular/Sass/CommonJS warningsnpx ng test draw --watch=false --progress=false --browsers=ChromeHeadlessCIpassed,30 SUCCESSgit diff --check origin/develop...HEADpassed
I found a couple of behavior regressions that should be fixed before merge. The main issue is preserving the old default solid fill behavior anywhere callers do not explicitly pass fillStyle. Also, GeometryStyleOptions in packages/draw/src/utils/geometry.ts should include fillStyle, otherwise public helpers like createGeometryElement(..., { fillStyle: 'hachure' }) are still rejected by TypeScript even though PlaitCommonGeometry now supports the property.
|
@nightt5879 Updated based on your feedback, and remove the special mock data about fillStyle, please see that again. |
nightt5879
left a comment
nightt5879
left a comment
There was a problem hiding this comment.
Reposting the review notes in English. I found two items worth confirming before approval: one possible missing fillStyle propagation path for UML class/interface rendering, and one small unused-import nit.
|
I am still working on it, I found some problems on table-backed scenarios. |
|
Updated the implementation based on the latest direction: this PR will not support fillStyle for tables or table-based elements for now, including UML class/interface and swimlane. Table cell fills are forced to use solid to keep the scope smaller and avoid expanding the current interaction/rendering complexity. fillStyle is currently only enabled for regular closed geometry elements. We can revisit table/cell-level fill styles later with a dedicated interaction and data model design. @nightt5879 Please help me review again! |
nightt5879
left a comment
nightt5879
left a comment
There was a problem hiding this comment.
I did another full review on the latest head.
The previous implementation concerns look addressed: the default solid behavior is preserved for drawGeometry / drawShape direct calls and table cell rendering, the setting panel is now scoped to supported non-table-backed closed geometry, FillStyle is narrowed to the explicit supported values, and the new tests cover the relevant rendering options.
Given the latest scope clarification, I’m okay with this PR only supporting fillStyle for regular closed geometry for now, while keeping table / swimlane / UML class-interface shapes on solid fills. Table-backed fill styles can be handled later with a dedicated model and interaction design.
No further blocking concerns from my side.
3 participants
Changes
This allows shapes to render using any
rough.jssupported fill pattern instead of defaulting to solid.