-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Offcanvas as component #29017
Conversation
de386d1
to
f4ea6ae
Compare
There was a problem hiding this 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
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? |
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 |
There was a problem hiding this 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.
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. |
@GeoSot you should first focus on request changes and after I'll help you for the unit tests don't worry 😉 |
Just have a look at the existing tests ( Other concerns:
|
I preferred to keep it simple and focus on functionality and minimalism. If it succeed to be merged we can add some styling
Only because the docs header has 1071 z-index. It can be transferred to docs.css |
Hi @GeoSot, do not hesitate to hit me up if you're stuck somewhere or if you did everything you planed to 😉 |
Thank u @Johann-S You may have a look on Js code and leave me some feedback. (Feel free to change things) |
@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 |
00dea32
to
7ce43da
Compare
7ce43da
to
03413f6
Compare
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 |
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. |
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. |
Also copyedit some docs wording
1c2582e
to
dcbd4c7
Compare
-- IOS devices, Android devices and Browsers on Mac, hide scrollbar by default and appear it, only while scrolling.
dcbd4c7
to
d0a4cbb
Compare
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. |
Took us long enough 😆 ❤️ |
Thank you George for the hard work ❤️💙 and for debugging BrowserStack tests 😄 |
Thank you all for your support on this MR. Thanks @XhmikosR for your patience |
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:
event.key
<div class="h4">
should be changed toh4
or similar?offcanvas
,off-canvas
etcFixes #27033, fixes #24718.
mdo's TODO: