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

Fix try haskell on Android #253

Merged
merged 2 commits into from
Feb 18, 2023
Merged

Conversation

andys8
Copy link
Contributor

@andys8 andys8 commented Feb 3, 2023

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
Collaborator

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

@andys8
Copy link
Contributor Author

andys8 commented Feb 3, 2023

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
Copy link
Contributor Author

andys8 commented Feb 3, 2023

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
Collaborator

The latest version seems to be fine on desktop.

extern.promptText("");
};

function clearScreen() {
Copy link
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


////////////////////////////////////////////////////////////////////////
// Report some message into the console
function report(msg, className) {
Copy link
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
Copy link
Contributor Author

andys8 commented Feb 5, 2023

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
Copy link
Contributor Author

andys8 commented Feb 15, 2023

@tomjaguarpaw Good to merge?

Copy link
Member

@TikhonJelvis TikhonJelvis left a comment

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
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
Copy link
Contributor Author

andys8 commented Feb 18, 2023

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

Exactly. Thanks for merging

@tomjaguarpaw
Copy link
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