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.

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

[button-group] Toolbar keynav fixes #366

Merged
merged 5 commits into from
May 9, 2017
Merged

[button-group] Toolbar keynav fixes #366

merged 5 commits into from
May 9, 2017

Conversation

tmorehouse
Copy link
Member

A couple of capslock typo fixes.

@pi0 pi0 merged commit c9e0c91 into bootstrap-vue:master May 9, 2017
@tmorehouse
Copy link
Member Author

tmorehouse commented May 9, 2017 •
edited

@pi0, I am wondering if the toolbar feature should be moved into it's own component, since it is really a wrapper for groups of button-groups?

See BS V4 example:
http://v4-alpha.getbootstrap.com/components/button-group/#button-toolbar

@tmorehouse tmorehouse requested a review from pi0 May 9, 2017 06:10
@pi0
Copy link
Member

pi0 commented May 9, 2017

Do you mean for accessibility features?

@tmorehouse
Copy link
Member Author

tmorehouse commented May 9, 2017 •
edited

Somewhat.

A toolbar will never have buttons as direct descendants. It will always have a child button group(s) and maybe input group(s). It might make it less confusing if the toolbar variant (and the accessibility stuff) was moved to its own wrapper component, which would then contain a slot for button groups and input groups.

@tmorehouse
Copy link
Member Author

maybe a b-button-toolbar component, or b-toolbar component.

@pi0
Copy link
Member

pi0 commented May 9, 2017

Never personally needed that but this is a official Bootstrap component and It would be useful :)

@tmorehouse
Copy link
Member Author

tmorehouse commented May 9, 2017 •
edited

Yeah. It is part of the button-group documentation of BS V4, but really it is its own wrapper/component that would have button-group and input-group's as children components. And then remove the toolbar prop option from the button-group

@tmorehouse
Copy link
Member Author

tmorehouse commented May 9, 2017 •
edited

I think the component wrapper should be b-button-toolbar, to follow the BS4 class of .btn-toolbar.

Removing the toolbar prop of b-button-group would be a breaking change, so it could be deprecated initially

@tmorehouse
Copy link
Member Author

I can whip up a new component in a few minutes.

valexiev added a commit to valexiev/bootstrap-vue that referenced this pull request May 9, 2017
@valexiev
Merge remote-tracking branch 'upstream/master'
230649f 
* upstream/master: (220 commits)
  [button-toolbar] docs (bootstrap-vue#368)
  ESLint
  New component b-button-toolbar (bootstrap-vue#367)
  [link] fix click event
  [docs] navbar styling
  Refactor link mixin
  [button-group] fix toolbar keynav (bootstrap-vue#366)
  [Docs] ScrollSpy directive JSFiddle
  [Link] Small fixes
  [link] Allow both router links and regular links to co-exist in same document (bootstrap-vue#365)
  [pagination] ARIA attributes + Keyboard navigation (bootstrap-vue#364)
  [dropdown-item] explicit component reference (bootstrap-vue#361)
  Additional ARIA on navs and dropdown (bootstrap-vue#358)
  ESLint
  [docs] ScrollSpy
  [scrollspy] SSR fix
  v0.15.6
  [modal] focusFirst timing tweak (bootstrap-vue#357)
  [scrollspy] documentation update
  [scrollspy] Documentation (bootstrap-vue#356)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pi0 pi0 Awaiting requested review from pi0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tmorehouse @pi0