Sid linking from clusters #1715
Conversation
rtorrero
force-pushed
the
sid-linking-from-clusters
branch
2 times, most recently
from
e5ffe57 to
bace198
Compare
rtorrero
force-pushed
the
sid-linking-from-clusters
branch
from
bace198 to
e86d2bf
Compare
rtorrero
requested review from
arbulu89,
dottorblaster,
CDimonaco and
jamie-suse
| systemType={linkData ? linkData.type : null} | ||
| sapSystemId={linkData ? linkData.sap_system_id : null} |
There was a problem hiding this comment.
You can use lodash's get function to get type and sap_system_id out of linkData
| ); | ||
|
|
||
| return ( | ||
| <SapSystemLink |
There was a problem hiding this comment.
mapping and returning JSX elements should require a key prop to be passed, can you do that?
| return foundInstance | ||
| ? { sap_system_id: foundInstance.sap_system_id, type: foundInstance.type } | ||
| : null; |
There was a problem hiding this comment.
You can directly return foundInstance without any fuss
There was a problem hiding this comment.
Hey @rtorrero ,
Some comments.
Besides that, I'm a bit surprised with the implementation, because as far as I see, it doesn't follow what our backlog ticket asks:
- Add link to SID colum in Cluster overview
- When SAP system or HANA Database is not registered, remove link from SID column in Cluster overview
(Anyway, I personally like more the current implementation behaviour, being honest)
| @@ -1,9 +1,11 @@ | |||
| import React from 'react'; | |||
| import { EOS_WARNING_OUTLINED } from 'eos-icons-react'; | |||
There was a problem hiding this comment.
Are we sure we want to change this component?
In the past, for this exact feature (adding a not registered tooltip to the link) we have created a new component, to not change the behaviour of this one (ClusterNodeLink).
Maybe we should follow the same pattern
There was a problem hiding this comment.
As I was looking how to add this I noticed that we have already a bunch of of *Link components that to basically very similar things. I'm leaning more towards keeping less components and add more when we need them. Wdyt?
| systemType={linkData ? linkData.type : null} | ||
| sapSystemId={linkData ? linkData.sap_system_id : null} | ||
| > | ||
| {singleSid} |
There was a problem hiding this comment.
What happens on multiple sids here?
Are they separated by spaces?
There was a problem hiding this comment.
I've used the tests to render how it would look like:
There was a problem hiding this comment.
After speaking with @dottorblaster we decided it might look better if we add the coma back. @jagabomb wdyt?
rtorrero
force-pushed
the
sid-linking-from-clusters
branch
from
caeb902 to
205711b
Compare
Description
This PR adds a tooltip for the SapSystemLink component so that when the linked SapSystem is not registered, a warning is shown.
It also adjusts the ClusterList to use this new component.
How was this tested?
Automated tests added