-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
#5858 - fix: Timeline node width made dynamic #6100
base: develop
Are you sure you want to change the base?
#5858 - fix: Timeline node width made dynamic #6100
Conversation
- Refactored timelineRenderer.drawEvents - Added DEFAULT_NODE_WIDTH of 150 - Used conf?.timeline?.width as a dynamic value, or default value if not provided - Added integration tests in cypress/integration/rendering/timeline.spec.ts to cover changes
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6100 +/- ##
==========================================
- Coverage 4.49% 4.47% -0.03%
==========================================
Files 383 384 +1
Lines 53925 54194 +269
Branches 596 623 +27
==========================================
Hits 2425 2425
- Misses 51500 51769 +269
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Hi @jgreywolf, I hope you're doing well! When you have some time, could you kindly take a look at my PR? Let me know if anything needs adjustment. Thanks! |
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.
Hello, I believe this is almost there, but as is, it's not functioning as intended. Below is the screenshot of a current simple test:
In order to fix this, we will need to also adjust the section
and task
width alongside the event
width. It's all configured in the same file.
I've done an implementation here, but since you've already PR'd feel free to adapt/grab what I've done.
Nit: I think that we should consider re-naming this config property to taskWidth
or eventWidth
, in order to increase readability. And update the timelineConfig
page to reflect that, and potentially remove all of the non-functional config variables.
- Added const DEFAULT_TASK_WIDTH of 150 - Refactored timelineRendered.draw - Added taskWidth, that used conf?.timeline?.width as dynamic value or default value if not provided - Refactored timelineRendered.drawTasks - Added taskWidth, that used conf?.timeline?.width as dynamic value or default value if not provided - Renamed nodeWidth variable in drawEvents to eventNodeWidth Thanks to @chriscconte for the helpful suggestions
|
Thanks for your feedback, @chriscconte ! I’ve applied your suggestions, and the timeline rendering looks much better now. |
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.
looks good!
📑 Summary
In timelineRenderer, the width of the timeline diagram nodes is hardcoded, making width configuration changes ineffective.
Resolves #5858
📏 Design Decisions
Extract the hardcoded value, defined here, into a constant and use it as the default if the node width is not specified in the timeline configuration.
I've added tests to verify the application of both the default and custom widths.
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSION
is used for all new features.pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.