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

[RFC] Joomla Dialog, implementation for Media, User Fields #40152

Closed
wants to merge 10 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Mar 19, 2023

Pull Request for Issue # .

Summary of Changes

Testing Instructions

  • Apply the patch
  • Run npm ci
  • Check that intro, full text images and author fields are working as before

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

User Field
Screenshot 2023-03-20 at 11 33 33

Media Field
Screenshot 2023-03-20 at 11 33 48

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev RFC Request for Comment labels Mar 19, 2023
@Quy
Copy link
Contributor

Quy commented Mar 19, 2023

users-dialog

@Fedik
Copy link
Member

Fedik commented Mar 20, 2023

Looks good, I still need to test.
Couple notes:
I call the asset "dialog" instead of joomla-dialog, because, well, it joomla already, like we have 'core', 'showon' etc ;)
I would prefer do not to have an extra css, it is very basic, I would do it as part of main template.
For width and height, would use default (defined by css) to keep all popup in consistent size, and set it in script when it need to be diferent.

- Move the src to media/system/ui
- Call the destroy
- change the Header
- rename the registry entry to dialog
@dgrammatiko
Copy link
Contributor Author

I call the asset "dialog" instead of joomla-dialog, because, well, it joomla already, like we have 'core', 'showon' etc ;)

I renamed them. I think it would be a good idea to have a system/ui folder and get also all the custom elements from the other repo here. I mean if there's a way to move the docs and the tests there's no reason to have multiple repos

I would prefer do not to have an extra css, it is very basic, I would do it as part of main template.

I disagree here. The script should provide some basic styling (mind I didn't copy everything from your PR, only the CE parts) and also the template has the ability to override the assets. Also we're doing that with the alerts and tabs..

For width and height, would use default (defined by css) to keep all popup in consistent size, and set it in script when it need to be diferent.

Removed them

@Fedik
Copy link
Member

Fedik commented Mar 20, 2023

The script should provide some basic styling

That is true, originaly I also wanted to do a separated css, but then I realise that the basic styling already comes with native dialog element ;)
Well, I know it is ugly, but it is usable. There only issue is iframe sizing.

@dgrammatiko dgrammatiko deleted the 4.3-dev-rfc-dialogs branch March 20, 2023 16:21
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 RFC Request for Comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants