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: Migrated the JS from template form to real JS #9966

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

MonalikaPatnaik
Copy link
Collaborator

Fixes #9106
Migrated the code of togglepassword.tt.js and user_form.tt.js scripts to /html/js/ as static JS files.

@MonalikaPatnaik MonalikaPatnaik requested a review from a team as a code owner March 20, 2024 16:05
@github-actions github-actions bot added JavaScript Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. 👥 Users labels Mar 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.68%. Comparing base (dc04d18) to head (1b47fd6).
Report is 266 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9966      +/-   ##
==========================================
+ Coverage   49.54%   49.68%   +0.14%     
==========================================
  Files          67       71       +4     
  Lines       20650    20980     +330     
  Branches     4980     5025      +45     
==========================================
+ Hits        10231    10424     +193     
- Misses       9131     9264     +133     
- Partials     1288     1292       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

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

Looks good, thank you @MonalikaPatnaik !

@MonalikaPatnaik
Copy link
Collaborator Author

@stephanegigandet could you pls merge this pull request.
Thanks!

@MonalikaPatnaik MonalikaPatnaik changed the title fix: Migrated the JS rom template form to real JS fix: Migrated the JS from template form to real JS Mar 27, 2024
@github-actions github-actions bot added the ⭐ top pull request Top pull request. label Apr 16, 2024
@MonalikaPatnaik
Copy link
Collaborator Author

@stephanegigandet could you pls merge this? I don't know why the unit test check is failing in this PR:(

@teolemon teolemon enabled auto-merge (squash) April 20, 2024 15:14
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

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

I updated the tests results and saw that the new scripts are loaded on all pages, I think it would make more sense to load them only on the sign in page and user edit page.

@@ -411,6 +411,8 @@ <h3 class="title-5 text-medium">[% lang('footer_discover_the_project') %]</h3>
<script src="[% static_subdomain %]/js/dist/display.js"></script>
<script src="[% static_subdomain %]/js/dist/stikelem.js"></script>
<script src="[% static_subdomain %]/js/dist/scrollNav.js"></script>
<script src="[% static_subdomain %]/js/dist/togglepassword.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Those 2 scripts are only needed on the sign-in and user page, can you load them only on those pages instead of all pages of the website?

Copy link

sonarcloud bot commented Apr 24, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud


<script>
$(function() {

$(document).ready( function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should not have been removed: when I check the "this is a pro account" checkbox, I'm not shown the warning anymore:

image

This is what we have in production:

image

@github-actions github-actions bot added the 💥 Merge Conflicts 💥 Merge Conflicts label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScript 💥 Merge Conflicts 💥 Merge Conflicts Site layout ⭐ top pull request Top pull request. Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. 🧪 tests 👥 Users
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Convert JS in Perl template into real JS
4 participants