Skip to content

Fix try haskell on Android - #253

Merged
tomjaguarpaw merged 2 commits into
haskell-infra:masterfrom
andys8:fix-try-it
Feb 18, 2023
Merged

Fix try haskell on Android#253
tomjaguarpaw merged 2 commits into
haskell-infra:masterfrom
andys8:fix-try-it

Conversation

@andys8

@andys8 andys8 commented Feb 3, 2023

Copy link
Copy Markdown
Contributor

Fix

Cherrypicked fix provided in chrisdone-archive/jquery-console#71.

Tested on desktop, iOS, and Android - with Android now working. There is also another comment confirming the fix.

Fixes #252

Review

I would welcome another person making sure "try it" is still working on desktop as before (comparing with currently deployed https://haskell.org).

Hint for review: I needed to change the listening host to 0.0.0.0 in order to be able to access the local dev server via my mobile device in the same network.

diff --git a/builder/site.hs b/builder/site.hs
index c5869dd..a56877b 100644
--- a/builder/site.hs
+++ b/builder/site.hs
@@ -66,7 +66,7 @@ main = mkContext >>= \ctx -> hakyllWith configuration $ do
 
 
 configuration :: Configuration
-configuration = defaultConfiguration{ providerDirectory = "site" }
+configuration = defaultConfiguration{ providerDirectory = "site", previewHost = "0.0.0.0" }
 
 parseTestimonialCompiler :: Compiler (Item Testimonial)
 parseTestimonialCompiler = do

Demo

Record_2023-02-03-19-54-48.mp4

@tomjaguarpaw

Copy link
Copy Markdown
Collaborator

For me, on desktop, it doesn't seem to allow entering space or numerals.

@andys8

andys8 commented Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

Oh, man, then it's not as easy as applying the suggested fix. Will take a look :D

@andys8
andys8 marked this pull request as draft February 3, 2023 19:14
@andys8

andys8 commented Feb 3, 2023

Copy link
Copy Markdown
Contributor Author

I pushed a change where copied the file from the fork over as is.

The result seems to be that it's working on Android and Desktop. On Android sometimes the number input seems to be a bit flaky, but better than before. Before I could confirm the same behavior you described.

Why didn't I do this in the first place? The fork has some formatting chances, so I thought I could only use the lines that fix logic. But also: The file of the fork has 845 lines while the file in the haskell-infra repository has only 750 lines. The difference is significant. I haven't looked into it and don't know if these differences are improvements, or code that was actively stripped away to run it on haskell.org.

I'd first test the current experience, if it's a fix at all, and then the difference should be investigated.

@andys8
andys8 marked this pull request as ready for review February 3, 2023 19:40
@tomjaguarpaw

Copy link
Copy Markdown
Collaborator

The latest version seems to be fine on desktop.

Comment thread site/js/jquery.console.js
extern.promptText("");
};

function clearScreen() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This change is coming from this commit: chrisdone-archive/jquery-console@3d405bf

Comment thread site/js/jquery.console.js

////////////////////////////////////////////////////////////////////////
// Report some message into the console
function report(msg, className) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This change is coming from this commit: chrisdone-archive/jquery-console@c8812e4

@andys8

andys8 commented Feb 5, 2023

Copy link
Copy Markdown
Contributor Author

I compared the changes other changes in jquery-console. I think - other than the actual fix - they are commits missing from upstream and formatting by the fix author. So I don't think these chances were intended to be missing, and therefore should be most likely fine to be merged, too.

Btw. ignoring white space helps a lot with github's diff.

image

@andys8

andys8 commented Feb 15, 2023

Copy link
Copy Markdown
Contributor Author

@tomjaguarpaw Good to merge?

@TikhonJelvis TikhonJelvis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, but would be great to have @tomjaguarpaw give it a quick test too.

@tomjaguarpaw

Copy link
Copy Markdown
Collaborator

When I tried it on desktop it was fine so let's give this a go. I don't really understand how it works, but if anyone spots any problems after merge we can always roll back.

@tomjaguarpaw
tomjaguarpaw merged commit 46c82d6 into haskell-infra:master Feb 18, 2023
@andys8

andys8 commented Feb 18, 2023

Copy link
Copy Markdown
Contributor Author

if anyone spots any problems after merge we can always roll back

Exactly. Thanks for merging

@tomjaguarpaw

Copy link
Copy Markdown
Collaborator

Thanks for contributing!

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.

Haskell "try it" not working on Android

3 participants