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

🧹 Clean up CMS javascript assets #3773

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

josemigallas
Copy link
Contributor

  • Reduce/Remove dependency from jQuery to remove technical debt
  • Remove mysterious functions from global scope
  • Reduce boilerplate
  • Remove old, broken and/or unused files and scripts
  • Remove duplicated files and code
  • Put files where they belong (dev portal ones under lib/developer_portal)

@josemigallas josemigallas self-assigned this May 2, 2024
@josemigallas josemigallas force-pushed the javascript_cms branch 5 times, most recently from d4d343f to 50a7d89 Compare May 9, 2024 09:53
@josemigallas josemigallas force-pushed the javascript_cms branch 3 times, most recently from c38d1ec to 98f2faf Compare May 13, 2024 07:50
@josemigallas josemigallas requested a review from a team May 13, 2024 13:37
@josemigallas josemigallas marked this pull request as ready for review May 13, 2024 13:37
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

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

I don't feel very capable of reviewing this but I checked out the branch locally and it seems to work fine for me.

@josemigallas how are you sure these are not breaking changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should remove this warning:

# FIXME: event handler defined at app/assets/javascripts/provider/admin/cms/templates.js:144 gets

<%= javascript_include_tag 'codemirror/modes/liquid' %>

<%= javascript_tag do %>
<% timeout = (Rails.env.development? or Rails.env.test?) ? 10000 : 3000 -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why different timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mayorova
Copy link
Contributor

Super hard to review this, as I don't understand some code well enough. But it seems that the features the changes affect work as expected.

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