Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Remove the RouterLink from the secondary-nav-section #465

Closed
NinaKammerlander opened this issue Jan 21, 2020 · 8 comments · Fixed by #734
Closed

Remove the RouterLink from the secondary-nav-section #465

NinaKammerlander opened this issue Jan 21, 2020 · 8 comments · Fixed by #734
Assignees
Labels
feature has-pr P2 Issue that is important to resolve as soon as possible pr: blocked This PR is blocked until a some condition (e.g. follow up issue) is resolved

Comments

@NinaKammerlander
Copy link
Contributor

NinaKammerlander commented Jan 21, 2020

Feature Request

The secondary-nav-section should work without a router.

Currently, the dt-secondary-nav-section contains a switch within its template where it defines whether the generated link should be either an external link (which uses the href attribute on the rendered html), or an internal link (which uses the routerLink directive).

Template reference from https://github.com/dynatrace-oss/barista/blob/master/components/secondary-nav/src/section/secondary-nav-section.html#L13-L22:

<a [href]="href" class="dt-secondary-nav-section-header" *ngIf="external">
    <ng-container *ngTemplateOutlet="content"></ng-container>
  </a>
  <a
    *ngIf="!external"
    [routerLink]="href"
    class="dt-secondary-nav-section-header"
  >
    <ng-container *ngTemplateOutlet="content"></ng-container>
  </a>

In general we are trying to avoid a hard dependency to the Angular Router, which is why we should think about removing this switch and let the consumer handle what linking mechanism they would like to choose.

@NinaKammerlander
Copy link
Contributor Author

@ffriedl89 can you please add more to the description? 😄

@tomheller
Copy link
Collaborator

tomheller commented Jan 23, 2020

I have updated the description to be a bit more telling about what is supposed to be the outcome.

@thomaspink
Copy link
Contributor

To resolve this we have to change the way the section works to something like:

<a dt-secondary-nav-section routerLink="/something">
    <dt-secondary-nav-section-title>
      Monitoring
    </dt-secondary-nav-section-title>
    <dt-secondary-nav-section-description>
      Setup and overview
    </dt-secondary-nav-section-description>
</a>
```

Internally the html of the section (expect for the template) has to move to the secondary nav itself (or an internal component). 

@thomaspink
Copy link
Contributor

We have to change the api and the way how the section work for that. Please hit me up if someone starts working on this issue.

@ffriedl89 ffriedl89 added P2 Issue that is important to resolve as soon as possible help wanted Extra attention is needed labels Jan 31, 2020
@ffriedl89
Copy link
Collaborator

@areknow We talked about this a while ago, and we would like to get this going again :) Would you be able to help us out here?
Any help is very much appreciated

@areknow
Copy link
Contributor

areknow commented Feb 3, 2020

@ffriedl89 sorry for the delay, I will get the PR ready this week

@github-actions github-actions bot added the no-issue-activity Issue is stale for more than 30 days label Mar 5, 2020
@dynatrace-oss dynatrace-oss deleted a comment from github-actions bot Mar 6, 2020
@tomheller tomheller removed the no-issue-activity Issue is stale for more than 30 days label Mar 6, 2020
@tomheller
Copy link
Collaborator

Issue is still relevant and should not be closed. @areknow would you still be interested to provide the fix for this one?

@areknow
Copy link
Contributor

areknow commented Mar 7, 2020

Hey @tomheller, I started the work and should be able to complete it next sprint 👍

@tomheller tomheller added has-pr pr: blocked This PR is blocked until a some condition (e.g. follow up issue) is resolved and removed help wanted Extra attention is needed labels Apr 9, 2020
areknow pushed a commit to areknow/barista that referenced this issue May 14, 2020
BREAKING CHANGE: the API has been simplified and the dependency on Router has been removed.
Fixes dynatrace-oss#465
ffriedl89 pushed a commit to areknow/barista that referenced this issue May 15, 2020
BREAKING CHANGE: the API has been simplified and the dependency on Router has been removed.
Fixes dynatrace-oss#465
ffriedl89 pushed a commit that referenced this issue May 15, 2020
BREAKING CHANGE: the API has been simplified and the dependency on Router has been removed.
Fixes #465
yngrdyn pushed a commit to yngrdyn/barista that referenced this issue Nov 9, 2020
BREAKING CHANGE: the API has been simplified and the dependency on Router has been removed.
Fixes dynatrace-oss#465
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature has-pr P2 Issue that is important to resolve as soon as possible pr: blocked This PR is blocked until a some condition (e.g. follow up issue) is resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants