Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #20 on issue 46 by chip...@gmail.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 A few notes on the CSS, after playing with it, that we'll need to address: 1. The red lines end up overflowing from the left-hand side to the right-hand side. This can be sort of prevented by adding 'position: relative; overflow: hidden;' to the parent td, but I'm not sure that fully worked. 2. If your window width isn't wide enough to show two columns of 80 characters, you'll never see red lines and never have any indication where the wrap point is. 3. Similarly, if text wraps due to being too long to show the full block of text (such as a long variable name) on the first line, and that text hits the 80 column boundary, then it'll wrap and you won't see where the red line was supposed to be within that text. -- You received this message because this project is configured to send all issue notifications to this address. You may adjust your notification preferences at: https://code.google.com/hosting/settings -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard-issues+unsubscr...@googlegroups.com. To post to this group, send email to reviewboard-issues@googlegroups.com. Visit this group at http://groups.google.com/group/reviewboard-issues. For more options, visit https://groups.google.com/d/optout.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #19 on issue 46 by trowb...@gmail.com: 80-column indicator in diff
view
http://code.google.com/p/reviewboard/issues/detail?id=46
Mike has some CSS that works for this:
table.sidebyside tr[line] > td:before {
content: '';
position: absolute;
transform: translateX(80ch);
border-left: 1px solid red;
height: 18px;
}
We'll have to figure out the best way to expose this.
--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
--
You received this message because you are subscribed to the Google Groups
"reviewboard-issues" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to reviewboard-issues+unsubscr...@googlegroups.com.
To post to this group, send email to reviewboard-issues@googlegroups.com.
Visit this group at http://groups.google.com/group/reviewboard-issues.
For more options, visit https://groups.google.com/d/optout.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #18 on issue 46 by matthew@kitware.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 I'm not sure user customization makes sense (or at least, is as important as it is being considered). If code length is part of the review process, 'what length' is going to be a property of the project, not the reviewer. Ergo, the logical place for it to be configurable is as a repository attribute. p.s. if you can rely on a fixed-width font, the CSS position of the margin can simply be specified in em units. If your DOM/CSS is sufficiently clever you can probably offload all the number crunching to the browser's render engine. (For that matter, if you *can't* rely on a fixed-width font, a margin is going to be ugly anyway, so...) An absolute position fixed-width div beneath the text but within the column element is probably sufficient. -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #17 on issue 46 by bernard3...@gmail.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 Speaking of cached content: This is often a project or company-wide setting, so maybe this could be a global setting (turn on, turn off, column size) would work for most cases and you can benefit from computing this server-side with the same content caching as normal. -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #16 on issue 46 by sambatis...@gmail.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 I'm very interested in this feature, or some way of turning off line wrapping / configure wrap margin. Would be a very useful feature for me and my team. -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #15 on issue 46 by bernard3...@gmail.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 One way I did something similar was to create a hidden div with 80 letters that had the same CSS styles and measure the width. I know some JS toolkits have APIs that can perform this measurement. (I only know ExtJS) -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #14 on issue 46 by shankar@gmail.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 The trick is to figure out *where* the 80th column is [...] For fixed-width fonts, this determination should be easy, shouldn't it? Style a vertical line at the appropriate horizontal offset, with the css being parameterized by the user preference? -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #13 on issue 46 by ju...@tuenti.com: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 Some companies use other number of columns (ie: 100) so this should be configurable also -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-issues@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #12 on issue 46 by bernard3000: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 If it is decided that this should be a user-level setting, why not cache the content with that setting as part of the cache key? So yes, that means a bigger cache, if there are a lot of variant of that setting and everybody is looking at everything. -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #11 on issue 46 by chipx86: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 Arun, this is one of those situations where an easy solution for one group is going to cause complaints for another. In large deployments, different teams may very well have different coding standards, and because of this we need a solution that is a bit more flexible. The trick is to figure out *where* the 80th column is. We need something that is fast but flexible. Doing it server-side is possible and may be speedy enough, but will become part of the cached content, so depending on how we let the users customize it (whether we choose to make it an admin setting or something flexible the user can change) it may or may not be an option. If doing it client-side, we need a way to quickly find the 80th column on each row, which can be slow. We also need to figure out the UI for showing that column. Background color could work. A thin line is a bit more difficult and looks odd on wrapped content. -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #10 on issue 46 by arunbalaji1985: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 Hi, We also follow certain standards like this in our company. And we have been using review board for quite some time now. Can the review board show margin after 80th column, or allow the admin to configure the limit based on the file extension. It could be a thin vertical line, or a slightly different background color. And/or, lines violating the limit could be highlighted. Any idea of this would be implemented? =Arun -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #9 on issue 46 by bernard3000: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 If anyone comes to implement something, I would like it be changeable to 120 (we have widescreen monitors now) :) For me, I think just putting a div line on top would be sufficient -- even if lines are "wrapped" (essentially ignore the wrapped portion, and do not show the line if the wrapping occurs "before" the 80 column mark). Would that work for others? Implementation: This could be done via javascript if the option is available. The javascript could decide to show the line or not "on-the-fly" (depending if that column is available without the wrapping (measure the size of "80 columns" in pixels with a "hidden div" with 80 letters of the same monospaced font). That way, the main document could stay unchanged if just the user settings is available somehow (external .js or additional ajax call). -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #8 on issue 46 by chipx86: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 It would need to be done in such a way where it's 1) fast, 2) customizable without regenerating the HTML, and 3) user-configurable. Not everyone at a company will necessarily have the same standards, so the user would need to be able to change this. It'd have to dynamically update the HTML. This is the slow, complicated part. Due to syntax highlighting, the text is broken up into many text fragments surrounded by tags. So knowing the correct place is a bit tricky. We'd be able to hack it by adding a vertical line or something, except then word-wrapping is an issue. I'm all for adding this feature, but someone needs to find a good way to do this and implement it. It's not on my todo list though. -- You received this message because you are listed in the owner or CC fields of this issue, or because you starred this issue. You may adjust your issue notification preferences at: http://code.google.com/hosting/settings -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
Re: Issue 46 in reviewboard: 80-column indicator in diff view
Comment #7 on issue 46 by timhollingsworth: 80-column indicator in diff view http://code.google.com/p/reviewboard/issues/detail?id=46 I'd love to have this. This issue is quite old - what is the chances it will be implemented? -- You received this message because you are listed in the owner or CC fields of this issue, or because you starred this issue. You may adjust your issue notification preferences at: http://code.google.com/hosting/settings -- You received this message because you are subscribed to the Google Groups "reviewboard-issues" group. To post to this group, send email to reviewboard-iss...@googlegroups.com. To unsubscribe from this group, send email to reviewboard-issues+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/reviewboard-issues?hl=en.
