NPM Resource Changed ? ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
9 May 2021

Pull Request for Issue #33652 .

Summary of Changes

Bug fixing

Testing Instructions

Click "Add module to the dashboard" on the Home Dashboard page. Look at JS Console.

Actual result BEFORE applying this Pull Request

Console errors

Expected result AFTER applying this Pull Request

No errors

Documentation Changes Required

avatar dgrammatiko dgrammatiko - open - 9 May 2021
avatar dgrammatiko dgrammatiko - change - 9 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 May 2021
Category JavaScript Repository NPM Change
avatar joomdonation
joomdonation - comment - 9 May 2021

Once we added the losing HTML code from this PR #32657 back, this PR would become invalid.

avatar dgrammatiko dgrammatiko - change - 9 May 2021
Labels Added: NPM Resource Changed ?
avatar joomla-cms-bot joomla-cms-bot - change - 9 May 2021
Category JavaScript Repository NPM Change Administration com_modules JavaScript Repository NPM Change
avatar dgrammatiko
dgrammatiko - comment - 9 May 2021

@joomdonation so I added back the alert part. BTW this PR also improves/simplifies the JS

avatar joomdonation
joomdonation - comment - 9 May 2021

Thanks @dgrammatiko . Will test it soon.

avatar joomdonation joomdonation - test_item - 9 May 2021 - Tested successfully
avatar joomdonation
joomdonation - comment - 9 May 2021

I have tested this item successfully on 6cbc292


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar joomdonation
joomdonation - comment - 9 May 2021

I have tested this item successfully on 6cbc292


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar joomdonation joomdonation - test_item - 9 May 2021 - Tested successfully
avatar joomdonation
joomdonation - comment - 9 May 2021

I have tested this item successfully on 7966444

Really works now. Maybe I should have coffee, too, before testing :D


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar PhilETaylor PhilETaylor - test_item - 9 May 2021 - Tested successfully
avatar PhilETaylor
PhilETaylor - comment - 9 May 2021

I have tested this item successfully on 7966444

Thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar ghazal ghazal - test_item - 9 May 2021 - Tested successfully
avatar ghazal
ghazal - comment - 9 May 2021

I have tested this item successfully on 7966444


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar richard67 richard67 - change - 9 May 2021
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 9 May 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33679.

avatar richard67 richard67 - change - 9 May 2021
Labels Added: ?
avatar richard67 richard67 - alter_testresult - 9 May 2021 - joomdonation: Tested successfully
avatar richard67 richard67 - alter_testresult - 9 May 2021 - PhilETaylor: Tested successfully
avatar richard67 richard67 - alter_testresult - 9 May 2021 - ghazal: Tested successfully
avatar richard67
richard67 - comment - 9 May 2021

@dgrammatiko With current 4.0-dev using Firefox on Windows I don't get any errors in browser console also without this PR applied. Can it be the errors have been already fixed with another, recently merged PR, maybe #32657 ? Or did they only occur on certain browsers?

avatar dgrammatiko
dgrammatiko - comment - 9 May 2021

@richard67 what's your ff version?

avatar richard67
richard67 - comment - 9 May 2021

88.0.1 (64-Bit) on Windows 10.

avatar dgrammatiko dgrammatiko - change - 9 May 2021
Labels Added: ?
Removed: ?
avatar dgrammatiko
dgrammatiko - comment - 9 May 2021

Screenshot 2021-05-09 at 16 10 59
88.0 here and looking good. Maybe this was out of sync (I just updated it)?

avatar richard67
richard67 - comment - 9 May 2021

@dgrammatiko I'd have no problem with merging this PR if it still makes sense, e.g. because improving code quality. I just like to understand that. It just seems there are no JS errors anymore to be fixed by it.

avatar dgrammatiko
dgrammatiko - comment - 9 May 2021

@richard67 oops my bad, misread what you wrote. So the problem was that there was a call to an element that didn't exist in the dom and calling foo.classList when foo is undefined will always throw an error. The code is bringing back the missing dom element but also introduces conditionals so it will not break if some dom element goes missing...

avatar richard67
richard67 - comment - 9 May 2021

@dgrammatiko Seems I was too much driven by the PR title "Fix the console errors" and so did not get why in addition the PR adds an alert. But I get it now, I've just tested. Without the PR, the "There are no modules matching your query." is missing, so it had to be added back here.

avatar richard67
richard67 - comment - 9 May 2021

Anyway code review looks good to me so far with my little js knowledge. Stuff is checked if defined before using it.

avatar richard67
richard67 - comment - 9 May 2021

Thanks!

avatar richard67 richard67 - close - 9 May 2021
avatar richard67 richard67 - merge - 9 May 2021
avatar richard67 richard67 - change - 9 May 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-09 14:44:55
Closed_By richard67
Labels Added: ?
Removed: ?

Add a Comment

Login with GitHub to post a comment