🌐 AI搜索 & 代理 主页
Skip to content

feat(b-sidebar): add prop backdrop-variant #5411

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.

Sign up for GitHub

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

Jump to bottom
Merged
jacobmllr95 merged 10 commits into from
May 20, 2020
Merged

feat(b-sidebar): add prop backdrop-variant #5411

jacobmllr95 merged 10 commits into from
May 20, 2020

Conversation

@Hiws
Copy link
Member

@Hiws Hiws commented May 20, 2020 •
edited by tmorehouse
Loading

Describe the PR

This PR adds an additional property backdrop-variant.
You can use the backdrop-variant prop to control the theme color variant of the backdrop.

PR checklist

What kind of change does this PR introduce? (check at least one)

  • Bugfix (fixes a boo-boo in the code) - fix(...), requires a patch version update
  • Feature (adds a new feature to BootstrapVue) - feat(...), requires a minor version update
  • Enhancement (augments an existing feature) - feat(...), requires a minor version update
  • ARIA accessibility (fixes or improves ARIA accessibility) - fix(...), requires a patch or minor version update
  • Documentation update (improves documentation or typo fixes) - chore(docs), requires a patch version update
  • Other (please describe)

Does this PR introduce a breaking change? (check one)

  • No
  • Yes (please describe since breaking changes require a minor version update)

The PR fulfills these requirements:

  • It's submitted to the dev branch, not the master branch
  • When resolving a specific issue, it's referenced in the PR's title (i.e. [...] (fixes #xxx[,#xxx]), where "xxx" is the issue number)
  • It should address only one issue or feature. If adding multiple features or fixing a bug and adding a new feature, break them into separate PRs if at all possible.
  • The title should follow the Conventional Commits naming convention (i.e. fix(alert): not alerting during SSR render, docs(badge): update pill examples, chore(docs): fix typo in README, etc). This is very important, as the CHANGELOG is generated from these messages, and determines the next version type (patch or minor).

If new features/enhancement/fixes are added or changed:

  • Includes documentation updates
  • Includes component package.json meta section updates (prop, slot and event changes/updates)
  • Includes any needed TypeScript declaration file updates
  • New/updated tests are included and passing (required for new features and enhancements)
  • Existing test suites are passing
  • CodeCov for patch has met target (all changes/updates have been tested)
  • The changes have not impacted the functionality of other components or directives
  • ARIA Accessibility has been taken into consideration (Does it affect screen reader users or keyboard only users? Clickable items should be in the tab index, etc.)

If adding a new feature, or changing the functionality of an existing feature, the PR's
description above includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@vercel
Copy link

vercel bot commented May 20, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/bootstrap-vue/bootstrap-vue/e0n3augc7
✅ Preview: https://bootstrap-vue-git-fork-hiws-featb-sidebar-add-prop-back-0e3287.bootstrap-vue.now.sh

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 20, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f13ec43:

Sandbox Source
recursing-wing-grpvo Configuration

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 20, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4f739e3:

Sandbox Source
sweet-ardinghelli-dz12e Configuration

@codecov
Copy link

codecov bot commented May 20, 2020 •
edited
Loading

Codecov Report

Merging #5411 into dev will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #5411   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          277       277           
  Lines         8875      8877    +2     
  Branches      2495      2495           
=========================================
+ Hits          8875      8877    +2
Flag Coverage Δ
#unittests 100.00% <100.00%> (ø)
Impacted Files Coverage Δ
src/components/sidebar/sidebar.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9172665...05b25b7. Read the comment docs.

@jacobmllr95 jacobmllr95 added Type: Enhancement PR: Minor Requires minor version bump labels May 20, 2020
@jacobmllr95
Copy link
Member

jacobmllr95 commented May 20, 2020

When thinking a bit about it we should have gone for the same logic as <b-modal>.
Render a backdrop by default and add hide-backdrop and no-close-on-backdrop props.

@tmorehouse
Copy link
Member

tmorehouse commented May 20, 2020

@jackmu95 although the majority of side bar usages I have seen in the wild are non-modal (i.e. don't show a backdrop by default, and allow user interaction with the page)

@Hiws
Copy link
Member Author

Hiws commented May 20, 2020

When thinking a bit about it we should have gone for the same logic as <b-modal>.
Render a backdrop by default and add hide-backdrop and no-close-on-backdrop props.

Possible. I remember the first time i opened the sidebar i tried clicking outside of it to close it, and was confused why nothing happened.
I've might just have grown used to that behavior from mobile.
So this was more to add back that behavior without having to have a visible backdrop.

@tmorehouse
Copy link
Member

tmorehouse commented May 20, 2020

There is actually an example of using a bootstrap modal as a side-modal twbs/bootstrap#27035 via some additional CSS/Classes

@tmorehouse
Copy link
Member

tmorehouse commented May 20, 2020

Perhaps the click-out handler could be used to close the modal when clicking outside (although it requires a click listener to be instantiated on the document body when the component is visible, and removed when hidden).

@jacobmllr95
Copy link
Member

jacobmllr95 commented May 20, 2020

@jackmu95 although the majority of side bar usages I have seen in the wild are non-modal (i.e. don't show a backdrop by default, and allow user interaction with the page)

This is a solid point.
I'am also fine with the the current solution.

@Hiws
Copy link
Member Author

Hiws commented May 20, 2020

Alternatively a backdrop-class prop could be made to pass classes to the backdrop, which might be more flexible in the long run, while still allowing the result that this PR produces.

But it would be less obvious that it's possible for the users, but that might not be the biggest issue.

@tmorehouse
Copy link
Member

tmorehouse commented May 20, 2020 •
edited
Loading

Or a backdrop-variant prop (defaulting to dark, which would be bg-dark class). Opacity it set via SCSS to make it translucent.

@jacobmllr95 jacobmllr95 changed the title feat(b-sidebar): add prop backdrop-transparent feat(b-sidebar): add prop backdrop-variant May 20, 2020
@vercel vercel bot temporarily deployed to Preview May 20, 2020 20:33 Inactive
@jacobmllr95 jacobmllr95 merged commit 4b0c163 into bootstrap-vue:dev May 20, 2020
@tmorehouse tmorehouse mentioned this pull request May 20, 2020
Merged
@jacobmllr95 jacobmllr95 mentioned this pull request Jul 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobmllr95 jacobmllr95 jacobmllr95 approved these changes

@tmorehouse tmorehouse tmorehouse approved these changes

Assignees

No one assigned

Labels

PR: Minor Requires minor version bump Type: Enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Hiws @jacobmllr95 @tmorehouse