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

Add a noscript tag to alert users when they have not enabled JS #1132

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

kinow
Copy link
Collaborator

@kinow kinow commented Mar 4, 2021

While testing the #700 PR, I refreshed the page a few times thinking there was something wrong in my container. Then I realized NoScript had prevented the JS of running in the Skosmos application.

Without JavaScript it's not able to search or navigate the application, so I have added in this PR the exact same noscript that comes by default in Vue.js applications (I believe React's message/placement is very similar).

That's useful for users with extensions such as NoScript. I know a few other friends using it here in New Zealand (even my wife now), but I guess in Europe there might be some too that are concerned with cookies/privacy/etc. So maybe it's worth alerting them to enable for Skosmos?

Cheers
Bruno

Screenshot from 2021-03-05 10-58-30

@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #1132 (5ce0da9) into master (6335b56) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1132   +/-   ##
=========================================
  Coverage     67.91%   67.91%           
  Complexity     1583     1583           
=========================================
  Files            32       32           
  Lines          3890     3890           
=========================================
  Hits           2642     2642           
  Misses         1248     1248           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6335b56...5ce0da9. Read the comment docs.

@osma
Copy link
Member

osma commented Mar 15, 2021

Thanks @kinow! I think this needs i18n support though, so Skosmos can show the message in the current UI language.

@kinow
Copy link
Collaborator Author

kinow commented Mar 15, 2021

Thanks @kinow! I think this needs i18n support though, so Skosmos can show the message in the current UI language.

Ah, true @osma. I've used the Twig macro to translate the string (is that all?). Then logged in to Transifex to see if I could add the entry there, but looks like someone else will have to do that first. I will add the Portuguese translation later once it's been added to Transifex.

Thanks!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.4% 2.4% Duplication

@osma osma self-assigned this Mar 16, 2021
@osma osma merged commit 5ce0da9 into NatLibFi:master Mar 16, 2021
@osma
Copy link
Member

osma commented Mar 16, 2021

I added the entries to the po files for English, Finnish and Swedish, which are maintained within the Skosmos repo (not Transifex), then merged this to master. Now Transifex should pick up the strings to translate from the skosmos_en.po file, so it should be possible to add the translations for e.g. Portuguese soon (I think that Transifex refreshes once per day).

Thanks again @kinow!

@osma osma added this to the 2.10 milestone Mar 16, 2021
@kinow kinow deleted the noscript branch March 16, 2021 07:34
@kinow
Copy link
Collaborator Author

kinow commented Mar 16, 2021

Will refer to your commit next time I need to add a text string 👍

Thank you @osma!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants