-
Notifications
You must be signed in to change notification settings - Fork 7
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
devenv: inject <noscript-tag> after build #42
base: main
Are you sure you want to change the base?
Conversation
@@ -122,6 +123,10 @@ in | |||
sed -i "s#Url.Builder.crossOrigin req.basePath req.pathParams#Url.Builder.absolute (\"api\" :: req.pathParams)#g" \ | |||
${config.devenv.root}/frontend/generated-api/src/Api.elm | |||
''; | |||
scripts.inject-noscript-tag.exec = '' | |||
sed -i "s|.*placeholder-no-script-tag.*|<noscript><div>Cannot use Flaskestry.dev without javascript</div></noscript>|" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give a better description than this? If you put yourself in the shoes of of someone who happened upon this site with a script blocker, your questions might be:
- What is Flakestry?
- Why does it require JavaScript (note the camelCasing) & what benefit will I get from allowing remote execution?
These questions should be minimally answered in the context of SPA where this is the only thing someone would see (no surrounding text, no link to an “About” page, etc.). Just telling someone to enable scripts while not giving the bare minimum for what & why is not a good user experience & will lead to page bounces.
… Also since this is a SPA, basic web crawlers & search engines will see only this text so why not give them something to actually index & show in their results?
In the linked issue this is supposed to close, I gave a sample of what might be better suited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
search engines / crawlers all support javascript these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not true: https://search.brave.com/search?q=flakestry.dev
No description or information about it since JS isn’t executed.
This works for me personally. I just want a reminder to whitelist the site instead of landing on a black screen. Also lets wait and see if @domenkozar will accept the pr. Keep in mind that this is a tag that is injected into markup after the build. Adding link tags in that context is not so clean. Not sure how a link to an 'about' page would help? It's all spa - the user wouldn't be able to see anything there until he whitelists the site |
The correct way to do this is to define a global layout in Elm and put it there. I'll hack on this during thaigersprint.org in two weeks. |
Ok then, I'll keep this open but feel free to close it once you have the global layout in. |
Informs users that they need to whitelist the site to view the SPA. Couldn't find a way to do within elm.
closes #27