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

Modal with Html5 dialog element #556

Merged
merged 19 commits into from
Dec 23, 2024
Merged

Modal with Html5 dialog element #556

merged 19 commits into from
Dec 23, 2024

Conversation

wuda-io
Copy link
Member

@wuda-io wuda-io commented Dec 16, 2024

Proposed changes

Change existing Modal Component so that it can be used with stadard Html5 dialog element.
See issue #525
Also fixes #528

Screenshots (if appropriate) or codepen:

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@wuda-io wuda-io mentioned this pull request Dec 16, 2024
8 tasks
@gselderslaghs
Copy link
Member

Its difficult to review, what HTML changes would be needed compared to the previous setup? Can you provide a code snippet that would work with this setup?

I notice in the documentation change inline javascript function handlers where implemented and the previous method of id referencing is abandoned, not really like this idea since it would introduce reduction to the semantics of the web pages more specific for screen readers and crawling bots since the reference between link and modal is currently only javascript based, I think we should keep the href=#anchor and id=anchor references by changing back from onclick to href from the point of view on web semantics

The approach with click handlers specifically targetting the modal instance using inline javascript method call handlers would also be more complex to setup in particular frameworks since there would be additional onclick handlers on the html element as of before we could just use the id referencing and the modals opening and closing works out of the box

@wuda-io
Copy link
Member Author

wuda-io commented Dec 19, 2024

Ok I understand, I think we could also add the href method back in, but then we need some javascript to initialize it then. I prefer to not initalize Components, because it often leads to problems when refreshing elements.

@gselderslaghs
Copy link
Member

Can you describe a scenario with where initialization fails because of refreshing the elements? Guess it's we should create an issue and fix them then..

I'm currently working on a project fork for Drupal Materialize - Google Material base where I'm replacing the old Materialize version with the new one, this is particular in the Drupal framework and would require elements initialization with JavaScript options rather then changing the HTML elements, since requiring attributes in the HTML element would require additional templating vs processing the options from PHP to JavaScript components options directly would not require any additional template updates, this is also the reason why I created #559

@wuda-io
Copy link
Member Author

wuda-io commented Dec 21, 2024

Can you describe a scenario with where initialization fails because of refreshing the elements? Guess it's we should create an issue and fix them then..

The problem with the init methods are, that they are often forgotten to be called and then people ask why does it not work etc. Also when the html-elements of the page are changed, they have to be re-initialized, which is also a soruce of many errors. So the best init method is no init method at all.

I think we can use the popover feature here.
https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/popover

Upgrade example would then be from
<button data-target="modal1" class="btn modal-trigger">Modal</button>
to

<button popovertarget="my-popover">Open Popover</button>
<div popover id="my-popover">Greetings, one and all!</div>
  1. replace data-target with popovertarget
  2. add popover attribute to all elements with class modal

Then we can also keep the tagnames (no dialog needed). Also it works without JS at all.

@wuda-io
Copy link
Member Author

wuda-io commented Dec 23, 2024

Ok I will merge it for now to make the release. I know it is not perfect, but we can keep up with fixes until it fits for broad mass. I think we should make a transition to the new Popover API. It is really cool and easy to use!
I think we should aim for 2.2.0 due to many changes.

@wuda-io wuda-io merged commit f2bfa3f into v2-dev Dec 23, 2024
0 of 2 checks passed
@wuda-io wuda-io deleted the modal-with-dialog branch December 23, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants