Add option to toggle ruler visibility#1479
Conversation
|
@zeroishero the issue is that the svg for the numbers is set with width=0 and height=0. This can be fixed by adding |
|
Note that the tests failed to because of the new |
0HyperCube
left a comment
There was a problem hiding this comment.
Works well, thanks for this contribution.
|
I just completed my code review and pushed some improvements split out into several commits. So you can learn from the changes I made, I'd encourage you to read over each of the commits. One other note for future consideration: when submitting PRs, please do so from a branch in your fork other than |
|
!build |
|
|
I was waiting for it to be reviewed and planned to make recommended changes myself. Anyway, thanks. I will look at changes. |
|
Yeah, sometimes we do code review by leaving comments, other times it's just easier to fix the things directly and push them. Having you fix the comments is better for your learning but more effort to describe on my part, so I took the more efficient route this time. I hope you can learn from the commit message + implementation to see. I also followed this up with one more commit which fixed a bug that I introduced (but didn't notice until after merging), so consider this change to be a part of the things you look at to learn through osmosis: 9224ed9 Thanks again! |
|
Hi @zeroishero, thanks again for this code contribution to the project! We're still hoping you will respond to the request to relicense this code. Please see #4208 ASAP, thank you! |
|
Hi @zeroishero, we're grateful of your past code contribution to Graphite, but unless we hear from you by the end of this weekend with agreement about relicensing your code to include the MIT license, we will begin steps to remove your code contributions so that you will no longer be a Graphite code contributor. We'd much rather avoid this outcome, so please respond now. To agree, simply log into your GitHub account, visit #4208, and copy-paste this into a comment: "I license my past and future contributions to Graphite under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option." If you're hesitant because you disagree or have questions, feel free to respond with another message; we'll do our best to answer your questions or concerns. More explanation is at the top of the linked thread if you're confused. So far, 164 contributors have agreed and you are among the 11% remaining whom we are still hoping to hear from before we begin removing your code from the project starting this Monday. (If you only see this later than Monday, your future belated response will still be appreciated.) Thank you for your cooperation and support of open source, and we hope you'll help us keep your valued contributions in-tact. |
Closes #1416
I added the menu, and made it send message to frontend to conditionally render the rulers element in svelte. I am using UpdateDocumentRuler and sending visible which is updated to rulersVisible. It works as intended but the markings and text is not being displayed. Here, as far as I know, the values necessary for computing markings and text is sent using UpdateDocumentRuler, and it has the data. I also tried updating spaces and interval and it did update it.
I watched the contribution video as guided by @Keavon and the process does match. I had tried making ToggleRuler message but it didn't send the data related to rulers along with it.