Contrib > Flyout permalink #1638
Conversation
Syncing with head
merge from base
Blackbaud-SteveBrush
requested review from
Blackbaud-BobbyEarl and
Blackbaud-PaulCrowder
Codecov Report
@@ Coverage Diff @@
## master #1638 +/- ##
==========================================
+ Coverage 99.98% 99.98% +<.01%
==========================================
Files 395 395
Lines 8139 8145 +6
Branches 1196 1198 +2
==========================================
+ Hits 8138 8144 +6
Misses 1 1
Continue to review full report at Codecov.
|
| @@ -37,4 +37,8 @@ export class SkyFlyoutAdapterService { | |||
| this.renderer.addClass(header.nativeElement, 'sky-flyout-help-shim'); | |||
| } | |||
| } | |||
|
|
|||
| public navigateToUrl(url: string) { | |||
| this.windowRef.getWindow().location.href = url; | |||
There was a problem hiding this comment.
Do we want this to always be the behavior? What if I'm already in the SPA where the destination link resides? Clicking the link would cause a full page refresh when instead I would think we'd want to do SPA navigation instead.
|
Friendly reminder about @Blackbaud-PaulCrowder's feedback. Related to that, can we talk through the expected functionality for the "View Record" button? If it's always just a URL that we'll navigate to, I don't think a button (semantically, not visually) is the right choice as users wouldn't be able to "Open in new tab." |
|
@Blackbaud-BobbyEarl That's a great point. I would also add, that even if this is an internal route, it should be an anchor too. I'll make those changes. |
|
@Blackbaud-BobbyEarl This is ready for another look. @Blackbaud-PaulCrowder Thoughts? |
There was a problem hiding this comment.
LGTM. Non-blocking feedback.
-
The anchor used when
permalink.routeis present is probably a good candidate forskyAppLink, but we'd have to work out exposing that outside of builder first (assuming we even want to). -
Showing a working example using the
route.command+route.extraswould probably be beneficial to add to the demo.
|
@Blackbaud-PaulCrowder has @Blackbaud-SteveBrush addressed your requested changes? |
| [ngIf]="config.permalink?.route"> | ||
| <a | ||
| class="sky-btn sky-btn-default sky-flyout-btn-permalink" | ||
| (click)="navigate(config.permalink.route)"> |
There was a problem hiding this comment.
There's no href attribute on the anchor tag here, so right-clicking and selecting "Open in new tab" won't work. Is there a way to use the routerLink directive here instead, which handles updating the href attribute?
Original contribution: #1520