Allows expanding the visible lines of code (Fixes #370) #409
Conversation
zedtux
force-pushed
the
feature/adds-expands-to-code-view
branch
4 times, most recently
from
7bd067e
to
6553899
Compare
|
@charliesome can I draw your attention to this PR? |
| %{<div class="code_linenums">#{formatted_nums.join}</div><div class="code">#{super}</div>} | ||
| code = '' | ||
|
|
||
| if true |
There was a problem hiding this comment.
Why is this condition here? It seems like this should be deciding whether there are more lines available.
There was a problem hiding this comment.
@RobinDaugherty sorry about that ... it is something I forget to finish, I'm updating it now.
| code << "<div class='code_linenums'>#{formatted_nums.join}</div>" | ||
| code << "<div class='code'>#{super}</div>" | ||
|
|
||
| if true |
There was a problem hiding this comment.
Same for this condition.
| @@ -19,8 +19,40 @@ def formatted_nums | |||
| } | |||
| end | |||
|
|
|||
| def expander_icon | |||
| '<svg aria-hidden="true" class="octicon octicon-unfold" ' \ | |||
There was a problem hiding this comment.
Please use heredoc syntax instead of this for multiline strings.
| @filename = filename | ||
| @line = line | ||
| @context = context | ||
| def initialize(filename, line, upperlines = nil, lowerlines = nil) |
There was a problem hiding this comment.
Prior to this change, the context argument received the number of lines of context, in other words the number of lines offset from line. Shouldn't still be the case now, instead of passing nil and causing the caller/instantiator to determine which line numbers should be included?
There was a problem hiding this comment.
The problem with the context is that it tells the amount of lines to be shown without taking care of the direction.
You want 5 lines more, fine, but 5 lines more in the upper or lower direction?
This class is used in the html_formatted_code_block method from the lib/better_errors/error_page.rb file in order to tells the amount of lines in the upper and in the lower direction (from the updated frame object).
So unless I miss understood your point, keeping the context in the initialiser doesn't make sense to me.
There was a problem hiding this comment.
My point was that the context argument gave the number of lines of context where the variables you added take line numbers, which might even be out of bounds.
|
Thank you @RobinDaugherty for your review, I'm my code now. |
zedtux
force-pushed
the
feature/adds-expands-to-code-view
branch
from
6553899
to
5323aa3
Compare
|
@RobinDaugherty the code has been updated. Please feel free to review it |
|
@RobinDaugherty can you please review my code? |
|
Sorry @zedtux I'm not able to answer quickly, but I'm able to spend time on this project once a week or so. |
|
Thank you @RobinDaugherty for your message, I'm available when you will be :) |
|
@RobinDaugherty a gentle reminder for my PR :) |
|
@RobinDaugherty and @charliesome a gentle reminder on this PR. I really think it's useful so I'm available for changing anything and get this thing merged :) |
|
@RobinDaugherty sorry for being asking again and again but can you please check the PR and let me know if there is anything I should change? Thank you. |
|
@RobinDaugherty can you please review this PR? |
|
@RobinDaugherty can you please review my PR? |
1 similar comment
|
@RobinDaugherty can you please review my PR? |
|
Can you please have a look @RobinDaugherty? This is really useful in my day to day work. |
|
@RobinDaugherty please, have a look at this PR, the changes have been made and should be fine to be merged. |
|
@charliesome and @RobinDaugherty can you please have a look at my PR? |
2 similar comments
|
@charliesome and @RobinDaugherty can you please have a look at my PR? |
|
@charliesome and @RobinDaugherty can you please have a look at my PR? |
|
Well ... it's now a long time ago I have oped this PR, with a review from @RobinDaugherty, who's now ignoring me and my PR ... What's the solution in order to get this merged? cc @charliesome |
|
Really interesting the way this PR goes. Whatever ... |
This PR implements the "Expand" icons like in Github (see #370). Clicking an expander calls the backend which returns only the code to be updated in the
.codediv so that the reste of the page stays as it is (no reset of the REPL).Here is a preview:
After having clicked the top expander: