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

feat(dropdown): optionally hide the dropdown toggle caret #1197

Merged
merged 4 commits into from Oct 14, 2017

Conversation

Projects
None yet
4 participants
@jon-walton
Contributor

jon-walton commented Oct 13, 2017

Hi

This allows the dropdown toggle to be hidden when the dropdown is not set to split. Useful for when the dropdown is just an icon.

usage

            <b-dropdown variant="link" hide-toggle size="sm">
              <template slot="button-content">
                <icon icon="avatar"></icon>
              </template>

              <b-dropdown-item @click="$store.dispatch('logout')">Logout</b-dropdown-item>
            </b-dropdown>

thanks!

@tmorehouse tmorehouse self-requested a review Oct 13, 2017

@tmorehouse

This comment has been minimized.

Member

tmorehouse commented Oct 13, 2017

Could you provide updated documentation for this feature?

@jon-walton

This comment has been minimized.

Contributor

jon-walton commented Oct 13, 2017

i wasn't sure where to put it, how is the props documentation created? or should it go in the dropdown/README.md somewhere?

thanks

@jon-walton jon-walton force-pushed the jon-walton:dropdown-toggle-optional branch from cf08d87 to 44295b1 Oct 13, 2017

@jon-walton

This comment has been minimized.

Contributor

jon-walton commented Oct 13, 2017

i saw what you meant now 😄 docs updated 👍

@codecov-io

This comment has been minimized.

codecov-io commented Oct 13, 2017

Codecov Report

Merging #1197 into dev will increase coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              dev    #1197     +/-   ##
=========================================
+ Coverage   32.86%   33.07%   +0.2%     
=========================================
  Files         109      109             
  Lines        2866     2866             
  Branches      891      920     +29     
=========================================
+ Hits          942      948      +6     
+ Misses       1547     1541      -6     
  Partials      377      377
Impacted Files Coverage Δ
lib/components/dropdown.vue 100% <ø> (ø) ⬆️
lib/components/form-input.vue 42.85% <0%> (+3.57%) ⬆️
lib/components/button.js 76.92% <0%> (+3.84%) ⬆️
lib/components/form-group.vue 76.66% <0%> (+6.66%) ⬆️
lib/components/button-group.js 100% <0%> (+33.33%) ⬆️
lib/components/embed.js 100% <0%> (+33.33%) ⬆️

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 485adbf...dc95ad1. Read the comment docs.

@jon-walton jon-walton force-pushed the jon-walton:dropdown-toggle-optional branch from 44295b1 to 121d8af Oct 13, 2017

@jon-walton

This comment has been minimized.

Contributor

jon-walton commented Oct 13, 2017

oh, i'll get the tests updated too

feat(dropdown):optionally hide the dropdown toggle
allow the dropdown toggle to be hidden when the dropdown is not set to
split. Useful for when the dropdown is just an icon

@jon-walton jon-walton force-pushed the jon-walton:dropdown-toggle-optional branch from 121d8af to cd10765 Oct 13, 2017

@@ -28,7 +28,7 @@ describe("dropdown", async () => {
// Without async iterators, just use a for loop.
for (let i = 0; i < dds.length; i++) {
Array.from(dds[i].children)
.find(node => node.tagName === "BUTTON" && node.classList.contains("dropdown-toggle"))
.find(node => node.tagName === "BUTTON" && node.id.includes("_BV_toggle_"))

This comment has been minimized.

@jon-walton

jon-walton Oct 13, 2017

Contributor

I had to change this to look for the id instead of the class name due to the new hide-toggle prop. i'm not familiar with how bootstrap vue generates the id so hopefully this is suitable?

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Here's how it is done:

If an ID is passed to the id prop of the <b-dropdown>, then that ID is used as the base and __BV_toggle_ is appended. If no ID is provided, then Bootstrap-Vue (on client only), we use the Vue unique UID (each vue VM is assigned a unique instance I on the client) to generate an ID string and __BV_toggle_is ppended to that. The ID generation is handled by the id mixin's safeId() method.

@@ -28,7 +28,7 @@ describe("dropdown", async () => {
// Without async iterators, just use a for loop.
for (let i = 0; i < dds.length; i++) {
Array.from(dds[i].children)
.find(node => node.tagName === "BUTTON" && node.classList.contains("dropdown-toggle"))
.find(node => node.tagName === "BUTTON" && node.id.includes("_BV_toggle_"))

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Here's how it is done:

If an ID is passed to the id prop of the <b-dropdown>, then that ID is used as the base and __BV_toggle_ is appended. If no ID is provided, then Bootstrap-Vue (on client only), we use the Vue unique UID (each vue VM is assigned a unique instance I on the client) to generate an ID string and __BV_toggle_is ppended to that. The ID generation is handled by the id mixin's safeId() method.

@@ -188,6 +188,26 @@ Create a split dropdown button, where the left button provides standard
<!-- dropdown-split.vue -->
```
## Hidden Toggle

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Technically this isn't really a hide toggle, as toggle refers to the button that triggers the dropdown, and you are just hiding the caret on the toggle button (by not applying the toggle class).

This probably should be called hide-caret, as removing the class dropdown-toggle stops the caret from being rendered (which is generated by CSS pseudo elements to add the dropdown/dropup caret)

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Actually as @mosinve suggested, no-caret would be better as the prop name.

@@ -70,6 +69,10 @@
type: String,
default: null
},
hideToggle: {

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Can we change this to hide-caret ?

This comment has been minimized.

@mosinve

mosinve Oct 13, 2017

Member

or no-caret

This comment has been minimized.

@tmorehouse

tmorehouse Oct 13, 2017

Member

Yeah, no-caret would be good

@tmorehouse tmorehouse changed the title from feat(dropdown):optionally hide the dropdown toggle to feat(dropdown): optionally hide the dropdown toggle caret Oct 14, 2017

<div>
<b-dropdown variant="link" size="sm" no-caret>
<template slot="button-content">
<icon icon="avatar"></icon>

This comment has been minimized.

@jon-walton

jon-walton Oct 14, 2017

Contributor

will using <icon /> here cause issues with the doc generation due to the component not being registered?

This comment has been minimized.

@tmorehouse

tmorehouse Oct 14, 2017

Member

It won't cause an issue, but it will not show up.

Since this is a live example (any html section that ends with a comment <!-- some-name.vue --> turns into a live example on the docs) it won't render correctly.

I would suggest just using a word, or an HTML entity code for a symbol, in place of the icon component (the docs to not use any icon libraries at the moment)

This comment has been minimized.

@jon-walton

jon-walton Oct 14, 2017

Contributor

ok thanks. updated to be a unicode emoji icon (magnifying glass)

@jon-walton

This comment has been minimized.

Contributor

jon-walton commented Oct 14, 2017

@tmorehouse hopefully i've addressed your comments. I pushed as another small commit, let me know when/if you want them squashed.

just one last question from me above regarding the use of <icon /> in the doc example

<template slot="button-content">
<icon icon="avatar"></icon>
&#x1f50d;

This comment has been minimized.

@tmorehouse

@tmorehouse tmorehouse merged commit 960877c into bootstrap-vue:dev Oct 14, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment