Display messages if JS or WebAssembly are not enabled.

[Alessandro-Battiato]

Resolves #23
Added HTML code to check if user's browser supports JavaScript and also added JS logic to alert the user if the browser does not
support WebAssembly

[not-my-profile]

Since you said that this is your first contribution to solve an issue, here some feedback:

• You can put Resolves #<issue-number>. in the PR description (where <issue-number> is the number of the GitHub issue you are solving) to make GitHub automatically link the PR to the issue (and merging the PR will automatically close the issue).

• Your commit implemented new features and also did some minor reformatting that was unrelated to the issue. Normally you want to separate formatting changes and function changes into separate commits so that they can be easily reviewed independently of each other (and also have descriptive commit messages attached). Here this doesn't really matter since the diff is so small but you may still want to try to separate these changes as a learning exercise.

  Regarding your formatting changes I don't really think the formatting changes in the HTML file improve anything. Regarding the JS file ... ideally this project would have some autoformatting set up that was checked in the CI ... currently we don't have that here.

• The commit message could be a bit more descriptive ... we don't just check if these are used but also alert the user ... so maybe Display messages if JS or WebAssembly are not enabled.

[Alessandro-Battiato]

First of all, I'm grateful you took your time to give me some feedback, so thanks for that :)

As for the issues you're pointing to, thanks again for the suggestion to include the issue number in the PR description, I didn't know that github worked like this!

Now I actually have a couple of questions to understand if there was a huge misunderstanding from my side regarding the issue but I need to give you context of what I've done.

Basically I added the no script in the HTML file as it appears only when your browser does not use JavaScript, and I thought this was the first task because indeed when disabling javascript in my browser's settings the message pops up. So question number one, if this is not the solution then what's the actual task?

Then regarding the formatting changes, could you explain in detail what you mean? If I understand correctly I should have first created the function logic and then commit it, and only then call it by also changing the original code?

Last question, I thought that the task about warning the user that the website uses the WebAssembly API should consist only in warning the user with an alert, and again what I've tested on my browser (specifically done this on Firefox though) was disabling the javascript options .wasm files and then the alert pops up immediately when the website is opened.

If this was not the solution and you mentioned an autoformatting setup using the CI, could you give me an example of what you meant?

Sorry if I'm asking too much but as you see I'm trying my best to learn and I'd be grateful if you again provided me more constructive criticism, which I'm grateful for :)

Have a nice day!

[not-my-profile]

thanks again for the suggestion to include the issue number in the PR description

You can edit the PR description by clicking on the three horizontal dots.

I need to give you context of what I've done

You don't need to ... I can see exactly what you have done by viewing the diff of your PR (e.g. by clicking on the "Files changed" tab on this PR). And yes you managed to solve the issue :) You did what was described :)

Then regarding the formatting changes, could you explain in detail what you mean?

Look at the diff of your PR or your commit. You will see changes like the following:

These are what I mean by formatting changes. It can very well be that you did not do them intentionally and your editor automatically formatted the file.

you mentioned an autoformatting setup using the CI

Sorry I didn't want to confuse you. Setting up such a setup is out of the scope of the issue that you are solving. By autoformatting i meant it would be nice if we set up something like Prettier. GitHub has a CI feature that would let us check that the files are correctly autoformatted ... but this is as I said out of the scope of the issue ... don't worry about it :)

I hope that cleared things up ... have a nice day as well :)

Note that reworking a PR so that the commits contain precisely the changes you want requires some know how of the git command-line interface which can be a bit overwhelming at first but I do recommend learning it in the long run. E.g. you could run git reset HEAD~ to undo the last commit while keeping your changes and then selectively stage the changes you want to commit (e.g. everything except the formatting changes) with git add -p ... and then commit with git commit. Git is very powerful if you start having PRs with multiple commits it's also worth it to learn git rebase -i, but for such a simple PR you don't need it. Once you have changed your commits as you want them you can force push them to the GitHub PR with git push -f.

[Alessandro-Battiato]

Thank you again for answering!

There's definitely a lot of stuff I need to learn, but if my PR solved the issue then I'll just edit the description on this one and mention the issue number, but I surely need to apply soon enough what you suggested me to do with Git as I'm still having trouble understanding how everything works, even though I watched some videos and already worked on some projects but I can already see the difference between working by myself and contributing even for a small issue on a small project.

Regarding the formatting changes you're correct I did not change anything intentionally except for adding the noscript, I use prettier on my editor and I'm so used to how Prettier does everything automatically that I didn't even notice the changes!

Thanks for the precious advice, this first contribution brought me much value :)

[waldyrious]

Hey @Alessandro-Battiato! I've left some comments inline, but I didn't realize that @not-my-profile had already reviewed the PR. Sorry for the duplication! In any case, it can be helpful to use those inline comments as separate threads, to allow the various suggestions to be discussed individually without a lot of quoting back and forth :)

[waldyrious]

Ok, I've pushed a new commit (6ca22d5) with a proposal for an alternative to the <noscript> approach to tell the user that JavaScript is needed to run the site.

Instead of inserting a paragraph at the top of the page and leaving the controls (textarea, button) available for interaction but unable to produce any effect, I decided to disable those elements, and activate them via JavaScript. Furthermore, the user message is provided as a placeholder in the textarea, which is where they would go to if they wanted to use the conversion functionality.

These changes ensure that the layout is kept the same on both states, improve the probablility that the user will notice the "JavaScript needed" message (since it is right in the place where they'd normally try to interact with the site), and overall make the site work more intuitively. @Alessandro-Battiato and @not-my-profile, please check this behavior and let me know what you think!

The one downside I have noticed is that Firefox seems to be caching the disabled/enabled state, so disabling JS and performing a simple reload will have those elements enabled, but incapable of functioning. A hard refresh (Ctrl+Shift+R) is required to actually load the page anew. This fortunately doesn't happen in Brave (a Chromium-based browser), and this situation should be rare in the first place, so I suppose it's not a big deal. But if anyone has ideas on how to bypass the browser's caching of element states, let me know!

[Alessandro-Battiato]

Ok, I've pushed a new commit (6ca22d5) with a proposal for an alternative to the <noscript> approach to tell the user that JavaScript is needed to run the site.

Instead of inserting a paragraph at the top of the page and leaving the controls (textarea, button) available for interaction but unable to produce any effect, I decided to disable those elements, and activate them via JavaScript. Furthermore, the user message is provided as a placeholder in the textarea, which is where they would go to if they wanted to use the conversion functionality.

These changes ensure that the layout is kept the same on both states, improve the probablility that the user will notice the "JavaScript needed" message (since it is right in the place where they'd normally try to interact with the site), and overall make the site work more intuitively. @Alessandro-Battiato and @not-my-profile, please check this behavior and let me know what you think!

The one downside I have noticed is that Firefox seems to be caching the disabled/enabled state, so disabling JS and performing a simple reload will have those elements enabled, but incapable of functioning. A hard refresh (Ctr+Shift+R) is required to actually load the page anew. This fortunately doesn't happen in Brave (a Chromium-based browser), and this situation should be rare in the first place, so I suppose it's not a big deal. But if anyone has ideas on how to bypass the browser's caching of element states, let me know!

I've tried the new functionalities on my machine, still using Firefox though so the elements are in fact enabled and not functioning, but still from a user perspective I am "forced" even before trying to type anything to read the placeholder which warns me that JavaScript is needed in order for the website to work properly, thus giving immediately the user the answer to the question " why won't it work".
But the placeholder option could probably have in my opinion a downside as compared to the noscript options, which happens in an extremely rare scenario where the user is plain ignoring every warning and every detail of the page and only writing inside the textarea without even reading the placeholder, making the latter disappear and having "no clue" as to what could be the cause of the problem that has broken the website.
Considering though this is indeed a rare behaviour, I like this option as well, but regarding the firefox problem I'm sorry to say that I have no idea how to solve that :(

[waldyrious]

But the placeholder option could probably have in my opinion a downside as compared to the noscript options, which happens in an extremely rare scenario where the user is plain ignoring every warning and every detail of the page and only writing inside the textarea without even reading the placeholder, making the latter disappear and having "no clue" as to what could be the cause of the problem that has broken the website.

That's a great point, I hadn't considered it! However, it shouldn't be an issue because besides adding the placeholder, the textarea is also made disabled by default, so the user should not be able to type into it. I just tested this in Brave, and even if the textarea has content, upon reloading the page with JS off, the textarea content is replaced with the placeholder (and the textarea is, as expected, disabled, so no new content can be added to make the placeholder go away).

IMO the fact that this issue can happen in Firefox is a bug in Firefox that we should consider working around; but we shouldn't distort the base implementation just to accomodate one browser that behaves awkwardly. Let's wait for @not-my-profile's thoughts, and if none of us comes up with a workaround, I suggest we open an issue to track this problem and leave its resolution for a future PR.

[waldyrious]

Friendly ping to @not-my-profile :)

[not-my-profile]

I cannot reproduce the Firefox bug you describe with Firefox 112.0.2.

Anyway I don't think adding this message in an input placeholder is a good idea because of accessibility reasons ... screen readers may not read placeholder text. So I think it's better to go with the <noscript> tag as I initially suggested.

[waldyrious]

Fair enough. I would still suggest the <noscript> message to be placed within the opening paragraph (bolded, perhaps, to call attention to it), and the rest of the changes (disabled form elements, placeholder text) could still be present as additional hints. Let me push a new commit with these changes.

[waldyrious]

Update: I re-added the <noscript>, with the content wrapped in a <mark> element to make it stand out. I actually went back on the placeholder to declutter the interface and avoid duplication of the warning.

I still left the activation of the form and apparition of the instructional placeholder to happen via script, so that they don't provide mixed messages to the user who has JS disabled.

Here's what it looks like without JS:

...and with JS:

WDYT @not-my-profile @Alessandro-Battiato?

[not-my-profile]

LGTM although I'd probably put the note in a separate paragraph to make it easier to spot.

[waldyrious]

Hmm. I see your point, but IMO with the bold and the <mark> syntax, there's enough semantic and visual signaling of this notice's importance, so I'll keep the current approach that's less disruptive in layout terms.

[waldyrious]

Ok, I've rebased the branch to split the changes into three atomic commits, and force-pushed it to update the PR. One of them is the code formatting, slightly adjusted to remove the line re-wrapping in index.xhtml (since the current wrapping was chosen deliberately), and to add a new '→" change in script.js. These differences can be seen here.

_{Update Sep 2023: I partially goofed the rebase: instead of just restoring the original line wrapping in index.hxtml, as mentioned above, I also inadvertently removed the <noscript> element. I have now re-added it in 154de20.}

As for the JS and WebAssebly support checks, nothing changed in the combined diff compared to the previous version of the PR before the force-push, but the various commits were combined and re-split into two commits, one for each tech being checked. Your authorship of the commits was preserved, @Alessandro-Battiato :)

Merging now — thanks both for the contribution and review!