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(login): Fix JS error on login grant page #41117

Merged
merged 1 commit into from Oct 26, 2023
Merged

fix(login): Fix JS error on login grant page #41117

merged 1 commit into from Oct 26, 2023

Conversation

callmemagnus
Copy link
Contributor

@callmemagnus callmemagnus commented Oct 25, 2023

Summary

HTMLCollection does not have a forEach method, so convert to array first.

Checklist

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

good catch, thank you!

@susnux susnux requested review from a team, artonge, Fenn-CS and sorbaugh and removed request for a team October 25, 2023 16:49
@susnux susnux changed the title fix(grant): remove JS error on Grant page fix(login): Fix JS error on login grant page Oct 25, 2023
@susnux susnux added bug 3. to review Waiting for reviews javascript labels Oct 25, 2023
@susnux susnux added this to the Nextcloud 28 milestone Oct 25, 2023
@susnux
Copy link
Contributor

susnux commented Oct 25, 2023

/backport to stable27

@susnux
Copy link
Contributor

susnux commented Oct 25, 2023

/backport to stable26

@joshtrichards
Copy link
Member

That code has been in-place since NC20 via #17683.

Why hasn't this lead to more bug reports... Or has it? 🤔

@susnux
Copy link
Contributor

susnux commented Oct 25, 2023

/backport to stable25

@@ -3,7 +3,7 @@ document.querySelector('form').addEventListener('submit', function(e) {
if (wrapper === null) {
return
}
wrapper.getElementsByClassName('icon-confirm-white').forEach(function(el) {
Array.from(wrapper.getElementsByClassName('icon-confirm-white')).forEach(function(el) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or wrapper.querySelectorAll which returns a NodeList.

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 is fine as querySelectorAll will have to do extra checks (the query could be anything) it will take much longer than you gain by omitting the array conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

(At least when I last benchmarked both with ~1k elements it took twice as long 😅 )

Closes #41098

Signed-off-by: Magnus Anderssen <magnus@magooweb.com>
@kesselb kesselb added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 26, 2023
@susnux
Copy link
Contributor

susnux commented Oct 26, 2023

Cypress is unrelated this seems to happen only on foreign repositories, somehow the 3rdparty submodule does not match up and then CI fails

@susnux susnux merged commit 082fa64 into nextcloud:master Oct 26, 2023
37 of 42 checks passed
@welcome
Copy link

welcome bot commented Oct 26, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@susnux
Copy link
Contributor

susnux commented Oct 26, 2023

/backport to stable26

@callmemagnus callmemagnus deleted the bugfix/grant__js-error branch October 26, 2023 15:19
@susnux
Copy link
Contributor

susnux commented Oct 26, 2023

/backport to stable25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: grant.js :: HTMLCollection does not have a .forEach method
7 participants