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

[4.0] Alerts custom element redo #34781

Merged
merged 6 commits into from Jul 22, 2021

Conversation

dgrammatiko
Copy link
Contributor

Redo of Pull Request #33234 .

This one should be treated as Release Blocker

(The alert code wasn't production-grade...)

Summary of Changes

The Alert Custom Element had some known issues and this PR is pulling a version that fixes all of them joomla-projects/custom-elements#179

Testing Instructions

Go to front end and backend and check that alerts are functioning as before (there are no styling changes, although the source is using CSS Vars for almost all parameters)

You can fire an alert using the browser's console with these snippets:

// Danger
Joomla.renderMessages({danger: ['Something happen']})
// Warning
Joomla.renderMessages({warning: ['Something happen']})
// Success
Joomla.renderMessages({success: ['Something happen']})
// Info
Joomla.renderMessages({info: ['Something happen']})

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Screenshot 2021-07-14 at 12 38 57
Screenshot 2021-07-14 at 12 39 12

Documentation Changes Required

The way that messages are rendered is exactly the same (only one internal change, not exposed to the API), so there is not anything to document.

@wilsonge DO NOT MERGE THIS WITHOUT MERGING THE OTHER PR CREATE A RELEASE AND CHANGE THE PACKAGE.JSON TO USE A VERSION!!!!

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Jul 14, 2021
@richard67
Copy link
Member

richard67 commented Jul 14, 2021

@dgrammatiko Npm still fails in drone.

Update: Would not be a problem as such because composer.lock will be updated anyway later when a new release has been made of custom_elements, but the failing npm causes other tests like the system tests not to run, so if it is possible to fix it, it would help with that.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jul 14, 2021

Npm still fails in drone.

I saw that but I have no other tricks here (we need to install an npm package using git but for some reason, the drone script doesn't support that). If you know another way please edit the packge.json directly

PS it should be the same reason why the other PR for the SCSS compiler also failed (spawn... )

PS it turns out that drone is using the node alpine container which doesn't have git (https://github.com/nodejs/docker-node/blob/fd130acf063b312355a5d88d51716db3ff34ae49/14/alpine3.11/Dockerfile)... I can propose a docker container based on node 14 alpine with git, this will probably solve the problem here but also in #33170

@wilsonge
Copy link
Contributor

Do we not need the close-text attribute on the hardcoded alert elements around core?

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jul 14, 2021

Are there any hard coded custom elements? If so yes, it's for a11y

@wilsonge there is only one instance with dismiss attribute and now it's also patched

@wilsonge
Copy link
Contributor

I have tested this item ✅ successfully on 2c92281

Navigated around the logins, stats plugin alert and javascript generated alerts (e.g. when trying to save an article with no title). All seemed fine afterwards


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

@wilsonge wilsonge merged commit faf9352 into joomla:4.0-dev Jul 22, 2021
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jul 22, 2021
@dgrammatiko dgrammatiko deleted the 4.0-dev-alerts-redo branch July 22, 2021 09:01
@dgrammatiko
Copy link
Contributor Author

Thanks!

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.

None yet

4 participants