Skip to content
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

Allow juxtaposed visual test #1168

Merged
merged 8 commits into from
Jul 9, 2019

Conversation

ashmaroli
Copy link
Contributor

Proof-of-concept for enhancing visual test by juxtaposing raw sample and highlighted result.

http://localhost:9292/<lexer>?juxtaposed=true

This is going to render an entirely different template to ease maintainability.
(However, the template has a header-n-footer divisions for completeness)

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jun 11, 2019
@pyrmont
Copy link
Contributor

pyrmont commented Jun 11, 2019

@ashmaroli Is this doing anything yet? I checked out the code but it looked (on my phone) like the raw text was just above the highlighted text. You said this PR is about juxtaposing (which, to be fair, it is) but for some reason, I imagined that what you meant was overlaying the samples.

It also occurs to me that this seems like an alternative approach to what @miparnisari was trying to address with #852.

@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 11, 2019
@ashmaroli
Copy link
Contributor Author

Is this doing anything yet?

The crux of this change is to place the raw code and highlighted result next to each other.

Juxtaposed Perl Test

overlaying the samples.

Nope.

alternative approach to #852

Is it @miparnisari ?

spec/visual/app.rb Outdated Show resolved Hide resolved
@pyrmont
Copy link
Contributor

pyrmont commented Jun 12, 2019

@ashmaroli Thanks for the quick answers. The graphic makes it clear. I do a lot of stuff on my phone so didn't see the difference at first :)

Given it's not overlaying the text (not sure why I thought that), it's less of a clean fit for fixing the concern in #852.

@miparnisari
Copy link
Contributor

This isn't an alternative to my PR, but maybe @ashmaroli wants to implement the functionality? It would be a warning (hopefully at the top so that it's very visible) if the 2 columns don't match, or if there is an error in the "highlighted" column.

@ashmaroli
Copy link
Contributor Author

if the 2 columns don't match

Programmatically, that'd be hard to do with a visual test. Will look into it..

@ashmaroli
Copy link
Contributor Author

@pyrmont This PR should complete my tasklist for improving the visual test app.. 😉

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

@ashmaroli I'm not sure if I'm pushing things too far but as you would have seen with ashmaroli#4 I've once again come in like a wrecking ball and excised a good chunk of your carefully handcrafted CSS. It looked to me no worse for wear but I only checked quickly; I may have missed something.

It was all of course in the aid of simplifying things.

I also thought the template should be renamed. Since this is a variation on the lexer.erb template, naming it something like lexer_juxt.erb seemed to communicate that more clearly. Let me know what you think!

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

PS. I must admit this is also pretty cool. Perhaps a future enhancement would get rid of the juxtaposed template and merely make this something that happens as the width of the <main> container gets to a certain threshold. Something to think about later, of course.

@ashmaroli
Copy link
Contributor Author

something that happens as the width of the <main> container gets to a certain threshold.

I didn't do that because juxtapositioning isn't always desirable.
Long line lengths in a sample will warrant horizontal scrolling which may be further complicated when line_numbers are rendered as well.
With this, there's always an option to fallback to the default view.

@pyrmont
Copy link
Contributor

pyrmont commented Jun 19, 2019

Oh yes, sorry to be clear, I agree it's not a good idea to do that in this PR. I just liked seeing the two divs side by side so much that it got me thinking that perhaps this should be the default (on sufficiently wide screens). Anyway, a discussion for another time :)

@pyrmont
Copy link
Contributor

pyrmont commented Jul 9, 2019

@ashmaroli I think I had erroneously left the author-action label on this one by mistake. It's ready to merge, right?

@ashmaroli
Copy link
Contributor Author

@pyrmont You left the author-action label since I had to merge ashmaroli#4 which was left idle in wait of the html-line-table formatter.

The time has now come for it all to fall into place.. Further discussion will be continued at the downstream PR.

@pyrmont
Copy link
Contributor

pyrmont commented Jul 9, 2019

@ashmaroli Ah, yes, now I remember. I should have labelled this dependent-pr but that's by the by now.

@pyrmont pyrmont added discussion-open and removed author-action The PR has been reviewed but action by the author is needed labels Jul 9, 2019
@pyrmont pyrmont added author-action The PR has been reviewed but action by the author is needed and removed discussion-open labels Jul 9, 2019
@pyrmont pyrmont merged commit 7879414 into rouge-ruby:master Jul 9, 2019
@pyrmont pyrmont removed the author-action The PR has been reviewed but action by the author is needed label Jul 9, 2019
@ashmaroli ashmaroli deleted the juxtaposed-visual-test branch July 10, 2019 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants