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.

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

[APM] Fix broken links #22592

Merged
merged 3 commits into from
Sep 6, 2018
Merged

[APM] Fix broken links #22592

merged 3 commits into from
Sep 6, 2018

Conversation

sorenlouv
Copy link
Member

Some links were missing the basepath

expect(panels[0].items[1].href).toBe(
'myBasePath/app/kibana#/management/elasticsearch/watcher/'
);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

The test here feel a little unnecessary. Let me know if you any suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I see what you mean but I think it's fine to be a little extra explicit with what we're worried about there with base paths.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv added the Team:APM All issues that need APM UI Team support label Sep 1, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

There are a couple other places where these similar links are broken:

  • getMlJobUrl() returns a link that isn't dynamic per base path, it's used in the Flyout here
    • could maybe use <KibanaLink> inside of that function and let the base path get added in one place, possibly?
  • You could possibly also use KibanaLink as a node passed to the Context Menu which seems better than using chrome in more places, except it doesn't seem like the context menu component wants you to do that very easily so what you have probably makes the most sense for now
  • <a href="/app/ml">Machine Learning jobs management page</a> is hard-coded in the Flyout as well, which could use a KibanaLink to fix I think

@sorenlouv
Copy link
Member Author

sorenlouv commented Sep 4, 2018

I could swear I had added base url for getMlJobUrl. Good catch!
I tend to agree with you that we shouldn't add addBasePath all over the place, but I'm hoping to get rid of KibanaLink (at least partly).

In the case with getMlJobUrl if we only need KibanaLink to add basePath, why not use addBasePath directly? EDIT: Actually, I agree with you. I'll remove addBasePath from getMlJobUrl.

@jasonrhodes
Copy link
Member

jasonrhodes commented Sep 4, 2018

Overall I think I prefer:

<SomeLinkAbstraction path="/some/path" />
<OtherComponent items={[
  <SomeLinkAbstraction path="/other/path" />,
  <Etc>
]} />

in lots of places, rather than:

<a href={someFn("/some/path")} />
<OtherLink path={someFn("/some/path")} />
<OtherComponent items={[
  { href: someFn("/other/path"), otherProp: 'whatever' },
  { etc: 'etc', etc: 'etc' }
]} />
// etc

but if there's history with KibanaLink we can move toward some better abstraction over time :)

@sorenlouv
Copy link
Member Author

@jasonrhodes I agree. I updated my comment (the illegal move - sorry! ;) )

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv merged commit 5f02f3e into elastic:master Sep 6, 2018
@sorenlouv sorenlouv deleted the fix-broken-links branch September 6, 2018 13:50
sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Sep 7, 2018
* [APM] Fix broken links

* Add missing basepaths

* Remove basepath from getMlJobUrl
@sorenlouv sorenlouv mentioned this pull request Sep 7, 2018
sorenlouv added a commit that referenced this pull request Sep 8, 2018
sorenlouv added a commit to sorenlouv/kibana that referenced this pull request Sep 8, 2018
* [APM] Fix broken links

* Add missing basepaths

* Remove basepath from getMlJobUrl
sorenlouv added a commit that referenced this pull request Sep 8, 2018
…22851)

* [APM] Fix broken links (#22592)

* [APM] Fix broken links

* Add missing basepaths

* Remove basepath from getMlJobUrl

* [APM] Fix ML links (#22820)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support v6.4.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants