-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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: allow specifying on which weekday a tickInterval should start #4634
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #4634 +/- ##
============================================
+ Coverage 45.28% 77.00% +31.71%
============================================
Files 52 143 +91
Lines 6633 14470 +7837
Branches 18 516 +498
============================================
+ Hits 3004 11143 +8139
+ Misses 3629 3222 -407
- Partials 0 105 +105
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Hi there @leinelissen , thank for you contribution! I like the idea of making diagrams on another day instead of Sunday, but the approach you are taking isn't right. Many users already created their diagrams relying on Sunday as a starting day; making such a change would affect the output of those diagrams, which isn't right. A better approach would be updating the grammar of the diagram to let users specify the starting day of the week, something like this:
Note that this is just a suggestion, we might need to modify it or something. |
I'm not sure if you are still interested in continuing working on it; if you are, you could rename the PR name and continue working. If not, please create an issue and close this PR. |
Hey @Yokozuna59, thanks for the feedback. I get that this is a breaking change, although I do feel like ISO points to Monday being the right starting day. Nonetheless, making an option out of this does sound like a good solution. I will take a stab at implementing this, although this will make the implementation more involved. I might need your help with some drafts before I manage to pull it off. |
Great!
Sure! I'm thinking of |
I've gone with |
Okay I think I've managed an implementation. If you have any feedback on it, please let me know! |
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.
Just minor changes.
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.
Sorry for the late review I was busy, more minor changes :)
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.
Final changes needed :)
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.
LGTM, thanks for contribution!
I'll leave the PR open for some time so that if someone from the team wants to highlight something.
@leinelissen, Thank you for the contribution! |
@leinelissen sorry there is something we have forgotten, weekends. If we changed the start of the week, that would mean we had new weekends, am I right? The result of I'll create an issue for it, feel free to work on it if you want. |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [mermaid](https://togithub.com/mermaid-js/mermaid) | [`10.2.4` -> `10.3.0`](https://renovatebot.com/diffs/npm/mermaid/10.2.4/10.3.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/mermaid/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/mermaid/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/mermaid/10.2.4/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/mermaid/10.2.4/10.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>mermaid-js/mermaid (mermaid)</summary> ### [`v10.3.0`](https://togithub.com/mermaid-js/mermaid/releases/tag/v10.3.0): 10.3.0 [Compare Source](https://togithub.com/mermaid-js/mermaid/compare/v10.2.4...v10.3.0) #### What's Changed ##### Features - Sankey diagrams by [@​nirname](https://togithub.com/nirname) in [https://github.com/mermaid-js/mermaid/pull/4502](https://togithub.com/mermaid-js/mermaid/pull/4502) - Feature/1838 actor creation destruction by [@​Valentine14th](https://togithub.com/Valentine14th) in [https://github.com/mermaid-js/mermaid/pull/4466](https://togithub.com/mermaid-js/mermaid/pull/4466) - Vertical branches in Git Diagram by [@​mastersibin](https://togithub.com/mastersibin) in [https://github.com/mermaid-js/mermaid/pull/4639](https://togithub.com/mermaid-js/mermaid/pull/4639) - Use JSON Schema to define and document `MermaidConfig` by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4112](https://togithub.com/mermaid-js/mermaid/pull/4112) - Remove the test checking whether the JSON Schema default config matched the old default config by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4610](https://togithub.com/mermaid-js/mermaid/pull/4610) - Fixes support of the macro `ContainerQueue_Ext` for C4 diagrams definition. by [@​kislerdm](https://togithub.com/kislerdm) in [https://github.com/mermaid-js/mermaid/pull/4577](https://togithub.com/mermaid-js/mermaid/pull/4577) ##### Bugfixes - Make quadrant chart options TypeScript types optional by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4602](https://togithub.com/mermaid-js/mermaid/pull/4602) - Remove double parsing by [@​nirname](https://togithub.com/nirname) in [https://github.com/mermaid-js/mermaid/pull/4587](https://togithub.com/mermaid-js/mermaid/pull/4587) - Fix flowchart tooltip typing bug by [@​lishid](https://togithub.com/lishid) in [https://github.com/mermaid-js/mermaid/pull/4562](https://togithub.com/mermaid-js/mermaid/pull/4562) - Bug/4590 allow notes identical to keywords by [@​ibrahimWassouf](https://togithub.com/ibrahimWassouf) in [https://github.com/mermaid-js/mermaid/pull/4597](https://togithub.com/mermaid-js/mermaid/pull/4597) - feat: allow specifying on which weekday a tickInterval should start by [@​leinelissen](https://togithub.com/leinelissen) in [https://github.com/mermaid-js/mermaid/pull/4634](https://togithub.com/mermaid-js/mermaid/pull/4634) - Split formatted markdown strings with unicode support. by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4470](https://togithub.com/mermaid-js/mermaid/pull/4470) - fix: Mind maps handles `-` signs in node ids/text by [@​knsv](https://togithub.com/knsv) ##### Chores - Remove all TypeScript enums and forbid them in ESLint by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4580](https://togithub.com/mermaid-js/mermaid/pull/4580) - refactor accessibility by [@​Yokozuna59](https://togithub.com/Yokozuna59) in [https://github.com/mermaid-js/mermaid/pull/4551](https://togithub.com/mermaid-js/mermaid/pull/4551) - chore: Reduce codecov pushes by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4604](https://togithub.com/mermaid-js/mermaid/pull/4604) - Run PR-labeler-config-validator only if config changes by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4607](https://togithub.com/mermaid-js/mermaid/pull/4607) - chore(deps): update all minor dependencies (minor) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4624](https://togithub.com/mermaid-js/mermaid/pull/4624) - Update all patch dependencies (patch) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4566](https://togithub.com/mermaid-js/mermaid/pull/4566) - Update all patch dependencies (patch) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4581](https://togithub.com/mermaid-js/mermaid/pull/4581) - Rename workflow jobs by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4574](https://togithub.com/mermaid-js/mermaid/pull/4574) - Removed unused code in state diagrams by [@​nirname](https://togithub.com/nirname) in [https://github.com/mermaid-js/mermaid/pull/4631](https://togithub.com/mermaid-js/mermaid/pull/4631) - chore(deps): update all patch dependencies (patch) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4623](https://togithub.com/mermaid-js/mermaid/pull/4623) - chore: remove unused `devDependency` on coveralls by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4641](https://togithub.com/mermaid-js/mermaid/pull/4641) - Allow entity diagram attribute names to start with asterisk by [@​ibrahimWassouf](https://togithub.com/ibrahimWassouf) in [https://github.com/mermaid-js/mermaid/pull/4588](https://togithub.com/mermaid-js/mermaid/pull/4588) - Bug/4592 fix new line padding class diagram by [@​ibrahimWassouf](https://togithub.com/ibrahimWassouf) in [https://github.com/mermaid-js/mermaid/pull/4633](https://togithub.com/mermaid-js/mermaid/pull/4633) - Fix graph not loading when the img loads too fast or fail to load by [@​pierrickouw](https://togithub.com/pierrickouw) in [https://github.com/mermaid-js/mermaid/pull/4496](https://togithub.com/mermaid-js/mermaid/pull/4496) - convert `cypress/helpers/util.js` to ts by [@​Yokozuna59](https://togithub.com/Yokozuna59) in [https://github.com/mermaid-js/mermaid/pull/4552](https://togithub.com/mermaid-js/mermaid/pull/4552) - build(deps-dev): bump word-wrap from 1.2.3 to 1.2.4 by [@​dependabot](https://togithub.com/dependabot) in [https://github.com/mermaid-js/mermaid/pull/4652](https://togithub.com/mermaid-js/mermaid/pull/4652) - chore(deps): update all minor dependencies (minor) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4663](https://togithub.com/mermaid-js/mermaid/pull/4663) - chore(deps): update all patch dependencies (patch) by [@​renovate](https://togithub.com/renovate) in [https://github.com/mermaid-js/mermaid/pull/4662](https://togithub.com/mermaid-js/mermaid/pull/4662) ##### Documentation - Sankey: Remove duplicated examples by [@​nirname](https://togithub.com/nirname) in [https://github.com/mermaid-js/mermaid/pull/4595](https://togithub.com/mermaid-js/mermaid/pull/4595) - Release docs by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4493](https://togithub.com/mermaid-js/mermaid/pull/4493) - Update latest news section by [@​huynhicode](https://togithub.com/huynhicode) in [https://github.com/mermaid-js/mermaid/pull/4495](https://togithub.com/mermaid-js/mermaid/pull/4495) - Fix Typo by [@​ryru](https://togithub.com/ryru) in [https://github.com/mermaid-js/mermaid/pull/4567](https://togithub.com/mermaid-js/mermaid/pull/4567) - Docs: add ChatGPT plugin blog post by [@​huynhicode](https://togithub.com/huynhicode) in [https://github.com/mermaid-js/mermaid/pull/4570](https://togithub.com/mermaid-js/mermaid/pull/4570) - Fix relative link to theme variables list by [@​ibrahimWassouf](https://togithub.com/ibrahimWassouf) in [https://github.com/mermaid-js/mermaid/pull/4573](https://togithub.com/mermaid-js/mermaid/pull/4573) - Fix docs:dev by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4598](https://togithub.com/mermaid-js/mermaid/pull/4598) - Docs: update link - "Join the Community" by [@​huynhicode](https://togithub.com/huynhicode) in [https://github.com/mermaid-js/mermaid/pull/4601](https://togithub.com/mermaid-js/mermaid/pull/4601) - Support docs:dev in docker by [@​nirname](https://togithub.com/nirname) in [https://github.com/mermaid-js/mermaid/pull/4599](https://togithub.com/mermaid-js/mermaid/pull/4599) - docs(flowchart): add documentation on multiple nodes style by [@​tomperr](https://togithub.com/tomperr) in [https://github.com/mermaid-js/mermaid/pull/4600](https://togithub.com/mermaid-js/mermaid/pull/4600) - Avoid downloading avtars everytime on docs:dev by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4603](https://togithub.com/mermaid-js/mermaid/pull/4603) - docs: Fix checkbox syntax by [@​guilhermgonzaga](https://togithub.com/guilhermgonzaga) in [https://github.com/mermaid-js/mermaid/pull/4646](https://togithub.com/mermaid-js/mermaid/pull/4646) - Fix the "Edit this page on GitHub" link in Vitepress documentation for the Mermaid Config pages by [@​aloisklink](https://togithub.com/aloisklink) in [https://github.com/mermaid-js/mermaid/pull/4640](https://togithub.com/mermaid-js/mermaid/pull/4640) - Support MERMAID_RELEASE_VERSION in docs. by [@​sidharthv96](https://togithub.com/sidharthv96) in [https://github.com/mermaid-js/mermaid/pull/4612](https://togithub.com/mermaid-js/mermaid/pull/4612) - Docs: update Latest News section by [@​huynhicode](https://togithub.com/huynhicode) in [https://github.com/mermaid-js/mermaid/pull/4655](https://togithub.com/mermaid-js/mermaid/pull/4655) - added Typora to integrations list by [@​kgilbert78](https://togithub.com/kgilbert78) in [https://github.com/mermaid-js/mermaid/pull/4666](https://togithub.com/mermaid-js/mermaid/pull/4666) - Docs: Corrects name of C4 link by [@​Incognito](https://togithub.com/Incognito) in [https://github.com/mermaid-js/mermaid/pull/4660](https://togithub.com/mermaid-js/mermaid/pull/4660) - Fix a typo by [@​gjtorikian](https://togithub.com/gjtorikian) in [https://github.com/mermaid-js/mermaid/pull/4396](https://togithub.com/mermaid-js/mermaid/pull/4396) #### New Contributors - [@​ryru](https://togithub.com/ryru) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4567](https://togithub.com/mermaid-js/mermaid/pull/4567) - [@​ibrahimWassouf](https://togithub.com/ibrahimWassouf) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4573](https://togithub.com/mermaid-js/mermaid/pull/4573) - [@​kislerdm](https://togithub.com/kislerdm) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4577](https://togithub.com/mermaid-js/mermaid/pull/4577) - [@​leinelissen](https://togithub.com/leinelissen) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4634](https://togithub.com/mermaid-js/mermaid/pull/4634) - [@​pierrickouw](https://togithub.com/pierrickouw) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4496](https://togithub.com/mermaid-js/mermaid/pull/4496) - [@​mastersibin](https://togithub.com/mastersibin) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4639](https://togithub.com/mermaid-js/mermaid/pull/4639) - [@​kgilbert78](https://togithub.com/kgilbert78) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4666](https://togithub.com/mermaid-js/mermaid/pull/4666) - [@​Incognito](https://togithub.com/Incognito) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4660](https://togithub.com/mermaid-js/mermaid/pull/4660) - [@​gjtorikian](https://togithub.com/gjtorikian) made their first contribution in [https://github.com/mermaid-js/mermaid/pull/4396](https://togithub.com/mermaid-js/mermaid/pull/4396) **Full Changelog**: mermaid-js/mermaid@v10.2.4...v10.3.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/levaintech/contented). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMS4wIiwidXBkYXRlZEluVmVyIjoiMzYuMTEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
📑 Summary
By default, d3 intervals start on Sunday as opposed to Monday. This is really unhelpful as Gantt charts are often used to play workweeks running from Monday to Friday. This means that all tasks that are aligned to workweeks are misaligned by a day when using
tickInterval 1week
.📏 Design Decisions
This pull request replaces d3's
timeWeek
withtimeMonday
, so that all week-based tickIntervals start on a Monday. Alternatively, I could implement multiple options for starting weekdays (using e.g.1week-monday
or1week-wednesday
), but I feel like weeks starting on Monday are a sensible default.📋 Tasks
Make sure you
develop
branch