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

Offcanvas as component #29017

Merged
merged 44 commits into from Mar 2, 2021
Merged

Offcanvas as component #29017

merged 44 commits into from Mar 2, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Jul 11, 2019

I did a first approach on an offcanvas component.
Αll of us, have used custom solutions, but maybe an official version, would be more stable.

Do you find it interesting? May I continue on this?

Preview: https://deploy-preview-29017--twbs-bootstrap.netlify.app/docs/5.0/components/offcanvas/

TODO:

  • we need to see if we can stop body scroll
  • maybe blur the bg if it doesn't have any performance hit
  • tweak blur, the current blur is too much IMO (XhmikosR)
  • Remove height from bottom offcanvas
  • Provide some default theming (white background, maybe a border or shadow like the modals)
  • Disable body scrolling by default.
  • measure FPS when blurring the bg (@XhmikosR i need guidance)
  • update offcanvas example to use this (also move it away from the experimental category)
  • reindent and fix missing end tags in the doc examples
  • switch to event.key
  • add more content in right, bottom examples
  • <div class="h4"> should be changed to h4 or similar?
  • Make sure the element is hidden and not offscreen when inactive
  • enforce focus, only on open element
  • Enable blur by default?
  • Revert the scrollbar decoupling
  • Consistent behavior of modals and offcanvas (eg. same way of setting a backdrop) Decouple BackDrop from modal #32439
  • non-text elements inside button related issue v5: Bootstrap tooltips don't appear after numbers of focusing #32372, Bootstrap 5 Tooltip with inline SVG #31646
  • Make sure all data attributes and any CSS vars are prefixed
  • Bring back the backdrop option, but keep it enabled by default
  • Make sure we aren't using any recently changed mr, etc classes
  • Should the backdrop transition be the same as the offcanvas one? Right now, backdrop shows instantly but finishes hiding after the offcanvas is closed .(GeoSot: it will be changed when Decouple BackDrop from modal #32439 is merged)
  • Decide about the name: offcanvas, off-canvas etc
  • Maybe rename the classes to consider RTL? Not sure if it's needed
  • Fix BrowserStack tests
  • spell checking from a native English speaker (@mdo?)
  • check accessibility (@patrickhlauke)
  • clean up/squash patches manually

Fixes #27033, fixes #24718.


mdo's TODO:

  • Finish and merge this PR
    • Undo the Offcanvas example change for now (we'll replace that later)
  • Close any other PRs
  • Iterate on the docs a bit more
  • Detach and de-dupe the backdrop in Decouple BackDrop from modal #32439

@GeoSot GeoSot requested review from a team as code owners July 11, 2019 23:47
@XhmikosR XhmikosR force-pushed the offcanvas-as-component branch 2 times, most recently from de386d1 to f4ea6ae Compare July 12, 2019 10:03
Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

Hi @GeoSot it's very interesting thanks! 👍
We are totally interested in this plugin

BTW you'll have to add unit tests

js/src/offcanvas.js Outdated Show resolved Hide resolved
js/src/offcanvas.js Outdated Show resolved Hide resolved
js/src/offcanvas.js Outdated Show resolved Hide resolved
js/src/offcanvas.js Outdated Show resolved Hide resolved
js/src/offcanvas.js Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Jul 12, 2019

Awesome! Haven’t dived into the code yet m, but obviously there’s a huge need for this. Can we ensure we have support for left, right, and bottom slide-in? Can we also make sure to stop the body scroll behind the sidebar?

@patrickhlauke
Copy link
Member

a crucial aspect of a properly working off-canvas is that focus should move to the offcanvas, remain trapped inside it until it's closed, and ideally the underlying document should be hidden via aria-hidden="true"

@patrickhlauke patrickhlauke self-requested a review July 12, 2019 19:37
Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

keyboard and assistive technology behavior must be addressed before this can be considered. conceptually, the off-canvas behaves pretty much like a dialog.

@GeoSot
Copy link
Member Author

GeoSot commented Jul 12, 2019

Hello Guys.

I really love this framework, and I want to contribute.

As you can see, I am a bit slower than the requested changes, and really I don't have any experience on unit testing.
So I would appreciate any help

@Johann-S
Copy link
Member

@GeoSot you should first focus on request changes and after I'll help you for the unit tests don't worry 😉

@MartijnCuppens
Copy link
Member

I don't have any experience on unit testing

Just have a look at the existing tests (js/tests/unit) and try to figure out how it works (for me, that worked 😛)

Other concerns:

  • I wouldn't nest the selectors (like &-right), we don't do that either for our modals. The full classname also makes searching for classes easier.
  • I would provide some default theming (like a default background) and create a theming section on the page with the colored components.
  • I spotted some inline styles like style="z-index: 1072;", we should avoid that.
  • Accessibility wise I would like to close the drawer when I press ESC key.

@GeoSot
Copy link
Member Author

GeoSot commented Jul 14, 2019

*I would provide some default theming (like a default background) and create a theming section on the page with the colored components.

I preferred to keep it simple and focus on functionality and minimalism. If it succeed to be merged we can add some styling

* I spotted some inline styles like `style="z-index: 1072;"`, we should avoid that.

Only because the docs header has 1071 z-index. It can be transferred to docs.css

@Johann-S
Copy link
Member

Hi @GeoSot, do not hesitate to hit me up if you're stuck somewhere or if you did everything you planed to 😉

@GeoSot
Copy link
Member Author

GeoSot commented Jul 21, 2019

Thank u @Johann-S

You may have a look on Js code and leave me some feedback. (Feel free to change things)

js/src/offcanvas.js Outdated Show resolved Hide resolved
@Johann-S
Copy link
Member

@GeoSot we switched from QUnit to Jasmine, so you'll have to update your branch and use Jasmine to write your unit tests, do not hesitate to take a look at other plugins

@GeoSot GeoSot force-pushed the offcanvas-as-component branch 2 times, most recently from 00dea32 to 7ce43da Compare July 29, 2019 22:13
@Johann-S
Copy link
Member

Johann-S commented Aug 1, 2019

Hi @GeoSot, I improved your unit tests, so the JavaScript part should be ok 👍 now we have to check the docs and maybe the SCSS ? @twbs/css-review

scss/_offcanvas.scss Outdated Show resolved Hide resolved
@MartijnCuppens
Copy link
Member

I preferred to keep it simple and focus on functionality and minimalism. If it succeed to be merged we can add some styling

Our other components also work out of the box without additional utilities for backgrounds (like the modal). For consistency, I would also like to see this here.

We should also ditch the heights for the bottom variant. Also there's some visible outline when the component is focused which need to be removed, like we do with the modals.

This PR also needs an approval from @patrickhlauke, to make sure everything is accessible.

@XhmikosR
Copy link
Member

XhmikosR commented Aug 2, 2019

Also, we need to see if we can stop body scroll like @mdo said above #29017 (comment)

And maybe blur the bg if it doesn't have any performance hit.

@XhmikosR XhmikosR added this to Inbox in v5 via automation Aug 2, 2019
GeoSot and others added 4 commits March 2, 2021 18:15
-- IOS devices, Android devices and Browsers on Mac, hide scrollbar by default and appear it, only while scrolling.
@XhmikosR
Copy link
Member

XhmikosR commented Mar 2, 2021

All right, this is pretty hard to rebase to clean up the patches into less commits, so I'll have to squash it into one patch.

Huge thanks to @GeoSot for sticking with us after all this time, to everyone else who contributed to the branch and to those who helped reviewing the PR!

We'll need to make some refactoring later to deduplicate some stuff, tweak doc examples, add a navbar offcanvas example, etc, but at least this should help a lot of people.

@XhmikosR XhmikosR merged commit 548be2e into twbs:main Mar 2, 2021
v5.0.0-beta3 automation moved this from Review to Done Mar 2, 2021
@mdo
Copy link
Member

mdo commented Mar 2, 2021

Took us long enough 😆 ❤️

@rohit2sharma95
Copy link
Collaborator

Thank you George for the hard work ❤️💙 and for debugging BrowserStack tests 😄

@GeoSot
Copy link
Member Author

GeoSot commented Mar 2, 2021

Thank you all for your support on this MR. Thanks @XhmikosR for your patience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta3
  
Done
Development

Successfully merging this pull request may close these issues.

Feature request: side Modals Drawer, sidebar, offcanvas nav