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] Native dialogs replacing the Bootstrap ones #39344

Closed
wants to merge 6 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Dec 2, 2022

Pull Request for Issue #32473 .

Summary of Changes

This is just a rough implementation nowhere near production!!!

The styling is extremely basic!!!

FWIW editing an article in com_content is the way to test this. That view went from 11 modals depending on Bootstrap to vanilla dialog with a little wrapper.

Also the DOM nodes went from
Screenshot 2022-12-02 at 12 35 15

to
Screenshot 2022-12-02 at 18 50 35

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Publishing tab, Select user
Screenshot 2022-12-02 at 19 39 02

Images & links, media select
Screenshot 2022-12-02 at 19 40 31

Content tab, tinyMCE buttons:
Screenshot 2022-12-02 at 19 43 34
Screenshot 2022-12-02 at 19 43 26

Content tab, CodeMirror buttons:
Screenshot 2022-12-02 at 19 45 43
Screenshot 2022-12-02 at 19 43 26

Toolbar buttons versions:
Screenshot 2022-12-02 at 19 46 17

Toolbar buttons preview:
Screenshot 2022-12-02 at 19 46 56

Toolbar buttons a11y check:
Screenshot 2022-12-02 at 19 47 36

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

@Fedik @brianteeman any comments here would be nice

@dgrammatiko dgrammatiko changed the base branch from 4.2-dev to 4.3-dev December 2, 2022 18:05
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev and removed Unit/System Tests labels Dec 2, 2022
@richard67 richard67 added Unit/System Tests and removed NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Dec 2, 2022
@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester and removed Unit/System Tests labels Dec 2, 2022
@brianteeman
Copy link
Contributor

(I had an issue with dvw not being supported but it was my browser not being on the version released on tuesday)

@@ -62,8 +62,8 @@ public function onDisplay($name)
return;
}

$link = 'index.php?option=com_content&view=articles&layout=modal&tmpl=component&'
. Session::getFormToken() . '=1&editor=' . $name;
$link = 'index.php?option=com_content&view=articles&layout=modal&tmpl=component&'
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if all of these links were constructed in the same way. eg
&editor=' . $name . '&' . Session::getFormToken() . '=1';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These plugins have huge issues as the editors plugins. Neither can use com_ajax, neither could support the newer Namespaced version, etc. So I guess someone should check the whole stack concerning the editor and their buttons.
Fwiw it should be nice to have a helper fn to build the urls, so I support the idea but there are more fundamental problems that need some proper solution instead of keep doing monkey patches...

@brianteeman
Copy link
Contributor

brianteeman commented Dec 2, 2022

Windows Google Chrome xtd-editor articles
When I have a full list of articles I get a triple scroll bar
image

When I have a shorter list of articles I get a double scroll bar
image

Could do with some spacing etc on the title (edit - i missed your comment about the styling being basic)
image

Close/Select on the media modal
image

Something has broken the spacing on the toollbar
image

@brianteeman
Copy link
Contributor

image

@Fedik
Copy link
Member

Fedik commented Dec 5, 2022

I will need some time to check it

@dgrammatiko
Copy link
Contributor Author

I will need some time to check it

Might be easier to check the code here: https://github.com/dgrammatiko/joomla-modal/tree/main/src
Basically there are 2 custom elements: one for the dialog element and another for an opener (button or a element) but events and other parts are missing atm

@Fedik
Copy link
Member

Fedik commented Dec 12, 2022

For begining we should draw some concept, how it should work.
We need popup that able to show embed page, inline html, and probably media for image/video preview (optional).
Also we need something for dialogs (prompt replacement)

I imagine it like that (if we choose the custom element path):

// Iframe
const popup = document.createElement('joomla-modal');
popup.type = 'iframe';
popup.url = 'blabla/url/';
popup.show(); 

// inline
const popup = document.createElement('joomla-modal');
popup.type = 'inline';
popup.popupContent = '<strong>blabla very strong text</strong>';
popup.show(); 

// image
const popup = document.createElement('joomla-modal');
popup.type = 'image'; // or 'video'
popup.url = 'blabla/url-to-image.jpg'; // or video.webm 
popup.show(); 

// prompt, can be added later
const popup = document.createElement('joomla-modal');
popup.type = 'prompt';
popup.popupContent = '<strong>Are you sure?</strong>';
popup.buttons = [{label: 'Yes', onClick: blablaClaaback}, {label: 'Not', onClick: blablaClaaback2}] // click on any of this also destroys the popup.
popup.show(); 

// In theory we can detect 'type' based on url or popupContent property

Showing popup:

// show() method automaticaly append itself to the document.body, when popup.parent === null;
popup.show();  

// user append popup where he want then show
blablaElement.addChildren(popup); 
popup.show();  

Closing and destroing popup:

popup.close(); // Just hides the popup, allow to show it again later
popup.destroy(); // Close (when open) and remove from DOM , kind of optional, or we use popup.parent.removeChild()

Events:

const popup = document.createElement('joomla-modal');
// After popup opened
popup.addEventListener('joomla-modal:shown', (event) => {});
// After popup closed
popup.addEventListener('joomla-modal:closed', (event) => {});

popup.show();  

The popup should give access to its content, so User can interact with it, kind of:

console.log(popup.popupBody);

Other things:

  • need an option to limit width height, for small/big popups
  • need an option to show or not the X close button, when it applicable
  • and something that I totaly forgot :)

All namings is subjective, can be changed, it just for ilustration.

One note I seen in code: do not use setAttribute, use propery instead:

popup.setAttribute('url', button.href); // This is potato
popup.url = button.href; // This is good

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Dec 12, 2022

I also agree with all with only couple of remarks:

One note I seen in code: do not use setAttribute, use propery instead:

Everything should be properties but also attributes should be allowed for strings, bool, num if we want a declarative solution also (we can sync them), maybe the modal wrapper doesn't need any attributes and those should only be on the <joomla-modal-button>...

need an option to show or not the X close button, when it applicable

True, also option for closing it when clicking on the backdrop (dialog element doesn't support that ootb)
A method to stop scrolling of the background
Something that kills the multiple scrollbars on a modal with an iframe

need an option to limit width height, for small/big popups

Lets use classes for the styling/sizing, I think I left a dummy clamp() line there but probably it is wrong

@Fedik
Copy link
Member

Fedik commented Dec 12, 2022

About setAttribute, I just looking how native elements works, example when we do img:

// we do
const img = new Image() // or document.createElement('img');
img.src = 'path/to/image'

// but not
img.setAttribute('src',  'path/to/image');

Of course both is valid, but that more clear I think.

@dgrammatiko
Copy link
Contributor Author

Of course both is valid, but that more clear I think.

I think you misunderstood me, what I meant was:

  • in js world (imperative) img.src = 'path/to/image'
  • in html (declarative) <joomla-modal-button iframe-src="path/to/iframe">

They both could exist in total harmony, it's just an added bonus if the property is named as the attribute and someone could use them interchangeably (eg input.type = 'bla' or input.setAttribute('type', 'bla'). But I won't fight for this as it's quite some work to reflect the values so let's use different names for props and attrs and call it a day.

@dgrammatiko
Copy link
Contributor Author

@Fedik so I'm gonna prototype a class (and not a custom element wrapper) for the modal factory and tackle the declarative cases later. I will ping you when something is reviewable (will move the code to Joomla-projects/custom-elements to have some tests)

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.

5 participants