Skip to content

[4.0] Decouple messages from core.js#32747

Merged
rdeutz merged 16 commits intojoomla:4.0-devfrom
dgrammatiko:—messages—4.0-dev
Apr 19, 2021

Hidden character warning

The head ref may contain hidden characters: "\u2014messages\u20144.0-dev"
Merged

[4.0] Decouple messages from core.js#32747
rdeutz merged 16 commits intojoomla:4.0-devfrom
dgrammatiko:—messages—4.0-dev

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Mar 19, 2021

Pull Request for Issue # .

Summary of Changes

  • This PR moves the core.js logic for rendering messages to another file
  • It uses import for bringing in the alert custom element (one less HTTP request)

Why?

Well, there is no way to override this part of the code without overriding the whole core.js, which is terrible as this is supposed to be the javascript API of the CMS. Bad architecture.

Depends/extends #32740

Testing Instructions

  • Apply the PR
  • try to login with the wrong credential in the front end and back end (should get an alert)
  • Try to upload an image in the media manager (you should also get an alert)

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Not really, the code is the same, we only moved it to an own file that is included from the respective layout (thus is properly overridable now)

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Mar 19, 2021
@dgrammatiko dgrammatiko changed the title ] [4.0] Decouple core.js from Bootstrap Mar 19, 2021
@dgrammatiko dgrammatiko changed the title [4.0] Decouple core.js from Bootstrap [4.0] Decouple messages from core.js Mar 19, 2021
@dgrammatiko dgrammatiko force-pushed the —messages—4.0-dev branch from 4099704 to cf40aaa Compare March 19, 2021 16:02
@dgrammatiko dgrammatiko force-pushed the —messages—4.0-dev branch from 33b286e to 93d43dc Compare March 20, 2021 15:47
@rdeutz rdeutz added this to the Joomla 4.0 milestone Mar 22, 2021
@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Please fix conflicts. Thanks.

@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Go to Smart Search.
Click Clear Index button.

Before PR:
32747-before

After PR (missing first message):
32747-after

@dgrammatiko
Copy link
Contributor Author

@Quy thanks for testing, it seems a bug with (animation transition [?]) alerts custom element as the HTML part was created correctly
Screenshot 2021-03-25 at 18 45 29

@dgrammatiko
Copy link
Contributor Author

@Quy 268a29e should fix it

@dgrammatiko dgrammatiko force-pushed the —messages—4.0-dev branch from 268a29e to da0eebe Compare March 25, 2021 18:44
@dgrammatiko dgrammatiko force-pushed the —messages—4.0-dev branch from da0eebe to cf88e16 Compare March 25, 2021 18:46
@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Fixed, however, the first message is colored blue and not green.

32747

@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Go to System > Redirects
Click Redirect System Plugin in the alert.

Alert should be red.
Modal does not open.

@dgrammatiko
Copy link
Contributor Author

Modal does not open.

Say hello to HTML sanitization, will check this

@dgrammatiko
Copy link
Contributor Author

@Quy fixed both, I have to alter also the other PR for the allowed attributes

@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Go to Media.
Click Delete button.

Alert should be red.

@dgrammatiko
Copy link
Contributor Author

Alert should be red.

So I thought that I could drop the old strings but I guess that will be B/C break, so they're back

@Quy
Copy link
Contributor

Quy commented Mar 25, 2021

Looks good!! I will test some more.

In the meantime, if you can look at these related issues: #30880 #29152

Thanks!

@dgrammatiko
Copy link
Contributor Author

@Quy #32868 should solve both the issues

@particthistle
Copy link
Member

I have tested this item 🔴 unsuccessfully on aa80075

Last moment test fail :(


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

@dgrammatiko
Copy link
Contributor Author

Last moment test fail :(

What failed?

@particthistle
Copy link
Member

Doh! Comment got lost as I was editing it.

  • The items on your test list all passed.
  • The items @Quy tested I also tested and they also passed. The note on the link colour on the redirect plugin IMO isn't an issue - in Beta 8 it's green. After applying your PR, it's also green. @Quy suggested it should be red. The modal there did work.
    image

The error encountered was in Joomla Update.

After testing, I went to reinstall Beta 7 to then update to Beta 8. Got to the point where I logged into Joomla Update and it was about to start unpacking the archive, and this AJAX error came up, rendering the update process broken.
image

@dgrammatiko
Copy link
Contributor Author

After testing, I went to reinstall Beta 7 to then update to Beta 8. Got to the point where I logged into Joomla Update and it was about to start unpacking the archive, and this AJAX error came up, rendering the update process broken.

This is not related to alerts AFAICT, something else is broken there as that is a native confirm not the alerts that this PR is patching

@particthistle
Copy link
Member

I have tested this item ✅ successfully on aa80075

Thanks for looking at that so quickly.
I've just tested the Joomla Update process again and it worked successfully

I think I'd broken it by clicking Apply Patch instead of installing the download package at the start of the process.


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

@Quy
Copy link
Contributor

Quy commented Apr 17, 2021

I have tested this item ✅ successfully on aa80075


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Apr 17, 2021
@Quy
Copy link
Contributor

Quy commented Apr 17, 2021

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 17, 2021
@rdeutz rdeutz merged commit 56f88c4 into joomla:4.0-dev Apr 19, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 19, 2021
@rdeutz rdeutz added this to the Joomla 4.0 milestone Apr 19, 2021
@dgrammatiko dgrammatiko deleted the —messages—4.0-dev branch April 19, 2021 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants