-
Notifications
You must be signed in to change notification settings - Fork 65
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
Fixes #37477 - Make host links be settings aware #571
Conversation
wrapper: ({ id, display_name: displayName }) => ( | ||
<a href={foremanUrl(`hosts/${id}`)}>{displayName}</a> | ||
wrapper: ({ name, display_name: displayName }) => ( | ||
<a href={foremanUrl(`${getHostsPageUrl(true)}/${name}`)}>{displayName}</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested the same mistake I did in yesterdays PR 🙃
I meant HOSTS_PATH
from import { HOSTS_PATH } from 'foremanReact/routes/Hosts/constants';
HOSTS_PATH is for /new/hosts
, for single hosts
getHostsPageUrl is for the experimental index host page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why is that a mistake? The link is built right and we do use it in other places as well:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think getHostsPageUrl
is supposed to be used this way anyway
export const getHostsPageUrl = displayNewHostsPage =>
displayNewHostsPage ? '/new/hosts' : '/hosts';
export const useForemanHostsPageUrl = () => {
const { displayNewHostsPage } = useForemanSettings();
return getHostsPageUrl(displayNewHostsPage);
};
useForemanHostsPageUrl
is using the setting displayNewHostsPage
which is for the index page, and not for the show page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created theforeman/foreman#10174 so foreman context will use the settings for both index and show page for host url
6c31f60
to
40f96a9
Compare
Thanks, @MariaAga, I've updated all the links in the wizard to use helpers from core. |
component="a" | ||
variant="link" | ||
target="_blank" | ||
href={foremanUrl(`${useForemanHostDetailsPageUrl()}${name}`)} // eslint-disable-line react-hooks/rules-of-hooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move useForemanHostDetailsPageUrl
out so it wont break react by accident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really... weird that this could break anything :/
Updated.
40f96a9
to
061d0a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks good but I didnt test it 👍
(cherry picked from commit 81a93e9)
No description provided.