Skip to content

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jun 2, 2021

Till this MR:

  • plugins that support dismiss action by clicking on a data-bs-dismiss trigger, can be dismissed ONLY if the trigger is inside the plugin html.

  • Alert, makes the difference. Alert on data-bs-dismiss click, tries to parse a data-bs-target element and in case in null, it fallback on the standard way, that assumes the trigger is inside the plugin html

This MR:

introduces the enableDismissTrigger helper function, that streamlines the functionality, using the idea of Alert component.

When 'dismiss' is clicked, it searches for getElementFromSelector(event.target) || this.closest(.${Plugin.NAME}) .

Result:

  1. The dismiss trigger can be either inside the component, or with the use of data-bs-target it can be anywhere in doc
  2. the dismiss trigger will respect the disable state and will handle preventDefault in case it is an anchor

The idea came from #33324

@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch from 9f13899 to 7df1c1f Compare June 3, 2021 23:33
@GeoSot GeoSot marked this pull request as ready for review June 3, 2021 23:45
@GeoSot GeoSot requested a review from a team as a code owner June 3, 2021 23:45
Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

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

How about defining enableDismissTrigger on the base-component? That way you could use it like Plugin.enableDismissTrigger(callback) and don't have to pass the plugin.

@GeoSot
Copy link
Member Author

GeoSot commented Jun 4, 2021

I don't know if it is ok to use it as static, as it will leave open the possibility to be called outside the code, causing fault usages

Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

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

bootstrap/js/src/modal.js

Lines 144 to 146 in 34218da

if (event && ['A', 'AREA'].includes(event.target.tagName)) {
event.preventDefault()
}

do we still need this here?

Copy link
Contributor

@alpadev alpadev left a comment

Choose a reason for hiding this comment

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

const rootElement = element ? this._getRootElement(element) : this._element

_getRootElement(element) {
return getElementFromSelector(element) || element.closest(`.${CLASS_NAME_ALERT}`)
}

This is covered by enableDismissTrigger too, isn't it?

@GeoSot
Copy link
Member Author

GeoSot commented Jun 4, 2021

bootstrap/js/src/modal.js

Lines 144 to 146 in 34218da

if (event && ['A', 'AREA'].includes(event.target.tagName)) {
event.preventDefault()
}

do we still need this here?

As we still support anchors with href triggers, I think yes.


const rootElement = element ? this._getRootElement(element) : this._element

_getRootElement(element) {
return getElementFromSelector(element) || element.closest(`.${CLASS_NAME_ALERT}`)
}

This is covered by enableDismissTrigger too, isn't it?

Is better to merge #33402 before this MR, because existing alert code, does some 'magic', and makes me nervous, using other code

After #33402 is merged, we won't have to solve this riddle

@alpadev
Copy link
Contributor

alpadev commented Jun 4, 2021

bootstrap/js/src/modal.js

Lines 144 to 146 in 34218da

if (event && ['A', 'AREA'].includes(event.target.tagName)) {
event.preventDefault()
}

do we still need this here?

As we still support anchors with href triggers, I think yes.

But this is done by enableDismissTrigger already? And from looking at the code there is no other call to hide() that passes an event?

@GeoSot GeoSot marked this pull request as draft June 4, 2021 16:24
@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch from 50e7b2e to 81f3ce8 Compare June 4, 2021 23:44
@twbs twbs deleted a comment from tungphan995 Jun 7, 2021
@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch 2 times, most recently from 9191063 to ef42102 Compare June 28, 2021 13:44
@GeoSot GeoSot marked this pull request as ready for review June 28, 2021 23:35
@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch 2 times, most recently from d5c0310 to f5b6c4d Compare July 9, 2021 13:02
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

As said in #33324, I'm a bit concerned about usability and accessibility. The refactor looks great, but use cases that it'd allow feel wrong.

Modal (and offcanvas) are supposed to make everything outside them inert while they're opened. Toast and alerts are IMHO valid use cases, but I'm not sure about this as a feature.

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

As said in #33403, this might need technical docs. Depending on this, we may need to add a callout regarding accessibility risks, but I guess we're pretty safe as long as we don't promote weird use cases.

@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch 2 times, most recently from 49b5fe9 to f74c336 Compare July 19, 2021 13:59
@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch 3 times, most recently from 6f3d544 to d998900 Compare July 22, 2021 22:20
@XhmikosR XhmikosR force-pushed the gs-merge-dismiss-functionality-to-method branch from 0256982 to 7304a4e Compare July 23, 2021 08:09
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Love the consolidation of code and docs!

@GeoSot GeoSot force-pushed the gs-merge-dismiss-functionality-to-method branch 2 times, most recently from 94e22ee to c68f809 Compare July 26, 2021 15:11
@XhmikosR XhmikosR force-pushed the gs-merge-dismiss-functionality-to-method branch from c68f809 to 5a81c5d Compare July 28, 2021 14:23
@XhmikosR XhmikosR merged commit 4bfd8a2 into main Jul 28, 2021
@XhmikosR XhmikosR deleted the gs-merge-dismiss-functionality-to-method branch July 28, 2021 14:39
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
* use a streamlined way to trigger component dismiss

* add documentation

Co-authored-by: XhmikosR <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants