Accept data-bs-body option in the configuration object as well #33248

GeoSot · 2021-03-02T23:36:39Z

Refactor offcanvas to accept config.
In order to use a simple configuration I separated bs-body option to bs-scroll and bs-backdrop

Preview

js/src/offcanvas.js

js/tests/unit/offcanvas.spec.js

site/content/docs/5.0/components/offcanvas.md

rohit2sharma95

Left few comments, except them LGTM 👍

js/src/util/index.js

js/tests/unit/offcanvas.spec.js

XhmikosR · 2021-03-04T18:41:40Z

js/src/util/index.js

@@ -153,6 +153,14 @@ const isVisible = element => {
  return false
 }

+const isDisabled = element => {
+  if (!element) {
+    return true


I'd return false here.

I am not sure. In code if an element not exists, you don't want to count it as valid. right?

But the check is for isDisabled. If the element doesn't exist then it can't be disabled?

Usually we check if a button/anchor/input (or another div) is disabled. Almost in any case it will not be null. The null check is for the 'just in case' situation
I can omitted if you find it more accurate

I prefer being stricter :) That being said, the logic seems off to me for the aforementioned reason.

another proposal is to throw an exception on line 157.
No-one has checked all these before, so if there is any other opinion I would like to know in order to finalize this

@twbs/js-review

IMO, if the DOM element does not exist it should not be considered as enabled. The absence of the DOM element is also like the disability 🤔

rohit2sharma95

LGTM 👍

js/src/offcanvas.js

js/tests/unit/util/index.spec.js

site/content/docs/5.0/components/offcanvas.md

js/src/util/index.js

Tweak jqueryInterface, add some more tests

GeoSot requested a review from a team as a code owner March 2, 2021 23:36

GeoSot mentioned this pull request Mar 2, 2021

Offcanvas — Remaining things #33244

Closed

4 tasks

GeoSot force-pushed the use-config-obj-on-offcanvas branch 2 times, most recently from a181c28 to e6ecb3e Compare March 3, 2021 00:25

rohit2sharma95 added js v5 needs tests labels Mar 3, 2021

rohit2sharma95 reviewed Mar 3, 2021

View reviewed changes

GeoSot requested a review from rohit2sharma95 March 3, 2021 22:03

rohit2sharma95 reviewed Mar 4, 2021

View reviewed changes

js/src/util/index.js Outdated Show resolved Hide resolved

js/tests/unit/offcanvas.spec.js Outdated Show resolved Hide resolved

js/tests/unit/offcanvas.spec.js Show resolved Hide resolved

XhmikosR reviewed Mar 4, 2021

View reviewed changes

GeoSot force-pushed the use-config-obj-on-offcanvas branch from 140cd7f to 663e16d Compare March 4, 2021 22:02

rohit2sharma95 approved these changes Mar 5, 2021

View reviewed changes

js/src/offcanvas.js Show resolved Hide resolved

js/tests/unit/util/index.spec.js Outdated Show resolved Hide resolved

site/content/docs/5.0/components/offcanvas.md Outdated Show resolved Hide resolved

rohit2sharma95 removed the needs tests label Mar 7, 2021

XhmikosR requested a review from mdo March 8, 2021 16:38

XhmikosR added this to Inbox in v5.0.0-beta3 via automation Mar 10, 2021

XhmikosR reviewed Mar 10, 2021

View reviewed changes

js/src/util/index.js Outdated Show resolved Hide resolved

XhmikosR moved this from Inbox to Review in v5.0.0-beta3 Mar 10, 2021

GeoSot requested a review from a team as a code owner March 10, 2021 08:48

XhmikosR force-pushed the use-config-obj-on-offcanvas branch from 9c4035b to bbb8b2f Compare March 10, 2021 12:59

GeoSot mentioned this pull request Mar 10, 2021

Dynamic tab should not show when triggered on disabled element #33257

Merged

GeoSot force-pushed the use-config-obj-on-offcanvas branch from bbb8b2f to 9ef3d25 Compare March 15, 2021 20:13

rohit2sharma95 approved these changes Mar 16, 2021

View reviewed changes

v5.0.0-beta3 automation moved this from Review to Approved Mar 16, 2021

XhmikosR added has conflicts and removed has conflicts labels Mar 16, 2021

XhmikosR force-pushed the use-config-obj-on-offcanvas branch from 65a810a to 475b920 Compare March 16, 2021 06:54

GeoSot and others added 2 commits March 16, 2021 08:57

Accept data-bs-body option in the configuration object as well

9f56bd5

Tweak jqueryInterface, add some more tests

Fix Markdown table formatting and tweak the wording on backdrop

1b35d63

XhmikosR force-pushed the use-config-obj-on-offcanvas branch from 475b920 to 1b35d63 Compare March 16, 2021 06:57

Merge branch 'main' into use-config-obj-on-offcanvas

9f7cd1f

XhmikosR approved these changes Mar 16, 2021

View reviewed changes

GeoSot merged commit ddf72bc into twbs:main Mar 16, 2021

v5.0.0-beta3 automation moved this from Approved to Done Mar 16, 2021

GeoSot deleted the use-config-obj-on-offcanvas branch March 16, 2021 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept data-bs-body option in the configuration object as well #33248

Accept data-bs-body option in the configuration object as well #33248

GeoSot commented Mar 2, 2021 •

edited

rohit2sharma95 left a comment

XhmikosR Mar 4, 2021

GeoSot Mar 4, 2021

XhmikosR Mar 8, 2021

GeoSot Mar 8, 2021 •

edited

XhmikosR Mar 10, 2021

GeoSot Mar 10, 2021

rohit2sharma95 Mar 14, 2021

rohit2sharma95 left a comment

Accept data-bs-body option in the configuration object as well #33248

Accept data-bs-body option in the configuration object as well #33248

Conversation

GeoSot commented Mar 2, 2021 • edited

rohit2sharma95 left a comment

Choose a reason for hiding this comment

XhmikosR Mar 4, 2021

Choose a reason for hiding this comment

GeoSot Mar 4, 2021

Choose a reason for hiding this comment

XhmikosR Mar 8, 2021

Choose a reason for hiding this comment

GeoSot Mar 8, 2021 • edited

Choose a reason for hiding this comment

XhmikosR Mar 10, 2021

Choose a reason for hiding this comment

GeoSot Mar 10, 2021

Choose a reason for hiding this comment

rohit2sharma95 Mar 14, 2021

Choose a reason for hiding this comment

rohit2sharma95 left a comment

Choose a reason for hiding this comment

GeoSot commented Mar 2, 2021 •

edited

GeoSot Mar 8, 2021 •

edited