jdaugherty commented on code in PR #15987:
URL: https://github.com/apache/grails-core/pull/15987#discussion_r3595619603
##########
dependencies.gradle:
##########
@@ -35,7 +35,7 @@ ext {
'javaparser-core.version' : '3.28.2',
'jline.version' : '3.30.6',
'jna.version' : '5.19.1',
- 'jquery.version' : '3.7.1',
+ 'jquery.version' : '4.0.0',
Review Comment:
jQuery 4.0.0 is a breaking major (drops IE and other legacy browsers,
removes long-deprecated utilities like `$.trim`, `$.type`, `$.proxy`,
`$.isFunction`, etc.), and because it's managed in both BOM maps, it isn't just
the freshly-generated welcome page that moves — any existing app that
references the jquery webjar without an explicit version will silently jump
3.7.1 → 4.0.0 when it upgrades to the Grails 8 BOM, and *their* JavaScript may
well use the removed APIs.
The templates themselves are fine (I checked the 4.0.0 webjar:
`dist/jquery.js` is at the same path so the `%` manifest reference still
resolves, and the ajax event alias methods used in `application.js` are still
present in the full build). But this needs a line in `upgrading80x.adoc`: note
the major bump, link the jQuery 4 upgrade guide, and mention that apps relying
on removed APIs can pin `org.webjars.npm:jquery:3.7.1` in their own build until
they migrate. Per the repo guidelines, user-facing changes like this shouldn't
merge without doc coverage.
##########
grails-profiles/web/skeleton/grails-app/assets/javascripts/application.js:
##########
@@ -17,4 +17,40 @@ if (typeof jQuery !== 'undefined') {
$(this).fadeOut();
});
})(jQuery);
-}
\ No newline at end of file
+}
Review Comment:
Not introduced here, but since this PR both bumps jQuery and touches this
file: the `$('#spinner').ajaxStart(...)` block just above has been dead code
since jQuery 1.9 — global ajax events fire only on `document`, so a handler
bound to `#spinner` never runs and the spinner in the layout never shows. If
we're keeping it, the working form is:
```js
$(document)
.on('ajaxStart', () => $('#spinner').fadeIn())
.on('ajaxStop', () => $('#spinner').fadeOut());
```
(Behavior unchanged in 4.0.0 — I checked the shipped `dist/jquery.js`.)
Applies to the forge copy of this file too; worth fixing in both while they're
being kept in lockstep, or deleting the block outright if the spinner isn't
worth keeping.
##########
grails-profiles/web/skeleton/grails-app/assets/javascripts/application.js:
##########
@@ -17,4 +17,40 @@ if (typeof jQuery !== 'undefined') {
$(this).fadeOut();
});
})(jQuery);
-}
\ No newline at end of file
+}
+
+// Navbar Controllers filter — rendered only when the list is long enough to
warrant
+// it (see the threshold in the layout). Filters the menu's [data-name]
entries in place.
+(function () {
+ function applyNavFilter(input) {
+ const scope =
document.querySelector(input.getAttribute('data-filter-scope') || '');
+ if (!scope) return;
+
+ const query = input.value.trim().toLowerCase();
+ let visible = 0;
+ scope.querySelectorAll('[data-name]').forEach((el) => {
+ const show = !query || (el.getAttribute('data-name') ||
'').toLowerCase().includes(query);
+ el.classList.toggle('d-none', !show);
+ if (show) visible++;
+ });
+
+ const empty = scope.querySelector('.nav-filter-empty');
+ if (empty) empty.classList.toggle('d-none', visible > 0);
+ }
+
+ document.addEventListener('DOMContentLoaded', () => {
+ document.querySelectorAll('.nav-filter-input').forEach((input) => {
+ input.addEventListener('input', () => applyNavFilter(input));
+ input.addEventListener('search', () => applyNavFilter(input));
+
+ const dropdown = input.closest('.dropdown');
+ if (!dropdown) return;
+ // Focus the field as the menu opens; clear it once the menu
closes.
+ dropdown.addEventListener('shown.bs.dropdown', () =>
input.focus());
Review Comment:
Focusing the filter as the menu opens is right on desktop, but on touch
devices it pops the on-screen keyboard the moment the Controllers menu opens,
covering half the list the user was about to scan. Suggest gating the autofocus
to pointer/hover environments:
```js
dropdown.addEventListener('shown.bs.dropdown', () => {
if (window.matchMedia('(hover: hover)').matches) input.focus();
});
```
Touch users can still tap the field (Bootstrap 5.3's dropdown exempts inputs
inside the menu from auto-close, so that works fine).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
