search

jquery / jqueryui.com

jQuery UI web site content
https://jqueryui.com
Other
132 stars 113 forks source link

Build: Fix copying top-level demos files #216

Closed mgol closed 2 months ago

mgol commented 2 months ago

The logic to copy files from demos was faulty for top-level files - it was copying them to the undefined subdirectory. This is now fixed.

Also, dot-files are no longer copied; we don't need the ESLint config copied, for example.

Also, the first commit contains some minor cleanup:

  1. Remove the externalDir variable that was computed but not used.
  2. Remove the local unused globalize variable.
  3. Declare the local external variable - previously, build code was overwriting the external global.
mgol commented 2 months ago

I cannot fully test this locally as demo embeds don't load in my local setup based on jquery-wp-docker but grunt deploy worked and prior to this fix a few files like search.js were copied to the undefined directory.

You can also verify that while https://jqueryui.com/resources/demos/search.js is a 404, https://jqueryui.com/resources/demos/undefined/search.js does exist. 🙂

I think we can merge it even without fully testing locally.

mgol commented 2 months ago

@Krinkle one thing - while this PR definitely fixes some copying issues, I don't think it will fix the demos in itself. jQuery UI deploy code deAMDifies JS the HTML and the resulting HTML doesn't use RequireJS at all. search.js still does, though, as only the HTML files are rewritten like that...

mgol commented 2 months ago

The proper fix will most likely also require (no pun intended) making search.js an UMD file. That's a jQuery UI change, though. This one would be useful to test locally to make sure it really works...

mgol commented 2 months ago

Actually, I could test it via a simple:

npx http-server -p 7001 dist/wordpress

and opening http://localhost:7001/resources/demos/autocomplete/multiple-remote.html in a browser. PR incoming...

mgol commented 2 months ago

The jQuery UI fix is available at https://github.com/jquery/jquery-ui/pull/2274. I tested both the in-repo setup and the jqueryui.com one.

mgol commented 2 months ago

As expected, https://stage.jqueryui.com/autocomplete/#multiple-remote now loads search.js but throws the require is not defined error. I checked that if I set up a local override for search.js, replacing it with what's on jQuery UI main, the demo starts working.

It should be working in production once jQuery UI 1.14.0 is released.