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

[link] Allow both router links and regular links to co-exist in same document #365

Merged
merged 9 commits into from
May 9, 2017
Merged

[link] Allow both router links and regular links to co-exist in same document #365

merged 9 commits into from
May 9, 2017

Conversation

tmorehouse
Copy link
Member

In some cases one would like to have both regular links (i.e. links to an in-page ID) and router links (routed by vue-router).

If to is not specified, then assume a regular local link (not handled by router).

Addresses #362 and #363 (I think... discussion is welcome)

@tmorehouse
Merge pull request #20 from bootstrap-vue/master
bf2afa7 
Resync with Master
@tmorehouse
Merge pull request #21 from bootstrap-vue/master
5baa351 
[modal] focusFirst timing tweak (#357)
@tmorehouse
Merge pull request #22 from bootstrap-vue/master
27c0a87 
Resync
@tmorehouse
Merge pull request #24 from bootstrap-vue/master
223e734 
Additional ARIA on navs and dropdown (#358)
@tmorehouse
[link] Allow router routes & regular href links
09c3071 
Allow both router links and regular links to coexist in same document

If `to` is not provided, then assume a regular local HREF
@tmorehouse
Update link.vue
534bf7d
@tmorehouse tmorehouse requested review from pi0 and mosinve May 9, 2017 04:14
@tmorehouse tmorehouse changed the title [link] Allow both router links ans regular links to co-exist in same document [link] Allow both router links and regular links to co-exist in same document May 9, 2017
pi0
pi0 reviewed May 9, 2017
View reviewed changes
lib/components/link.vue Outdated
@@ -1,6 +1,6 @@
<template>
<!-- When VueRouter is available -->
<a v-if="routerAvailable"
<a v-if="routerAvailable && to"
Copy link
Member

Choose a reason for hiding this comment

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

Can't we refactor && to into computed property?

pi0
pi0 reviewed May 9, 2017
View reviewed changes
lib/components/link.vue Outdated
@@ -52,16 +52,12 @@
},
_to() {
if (!this.routerAvailable || this.disabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Then this condition won't happen as routerAvailable is false when no to prop exists :))

pi0
pi0 reviewed May 9, 2017
View reviewed changes
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

.

@tmorehouse
Copy link
Member Author

@pi0, check the new changes and see what you think?

@pi0
Copy link
Member

pi0 commented May 9, 2017

LGTM :)

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

Hopefully this doesn't break something else :P

@pi0
Copy link
Member

pi0 commented May 9, 2017

Something is breaking Nav links. (Documentation website is broken)

@tmorehouse
Copy link
Member Author

Is documentation site using latest release or master?

@pi0
Copy link
Member

pi0 commented May 9, 2017

Testing locally on master. I think condition should be return this.$router && this.to :))

@tmorehouse
Copy link
Member Author

Agh, lol.. an easy fix (I hope)

@pi0
Copy link
Member

pi0 commented May 9, 2017

Yep, i'll publish fix soon

@MarZab
Copy link

MarZab commented May 9, 2017 •
edited

Shouldn't in the case neither href or to provided, the link be rendered as <a role="button>" since bootstrap does support this troughout? Related to: #362

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)
  ...
@tmorehouse
Copy link
Member Author

tmorehouse commented May 9, 2017 •
edited

BS V4.alpha.6 .dropdown-items support both using links or buttons (although there are slight styling issues between the two font-wise, which will be fixed in beta release).

Maybe the dropdown-item component should create a <button class="dropdown-item"></button> when both href and to are falsy? rather than using a link tag?.

@tmorehouse tmorehouse deleted the router-links branch May 10, 2017 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pi0 pi0 pi0 left review comments

@mosinve mosinve Awaiting requested review from mosinve

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
3 participants
@tmorehouse @pi0 @MarZab