-
Notifications
You must be signed in to change notification settings - Fork 64
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
Resource tab optimization #1435
Changes from 21 commits
360e920
b060909
f66ace8
0f725de
41b0aa2
c16159d
d6a7d8e
862b09e
9f9fa98
990535d
ffc7b1f
0c28d0d
cb55806
7e5812f
56c9b9b
ee2e804
4a47bd5
c1dc3ec
704fdde
17edc4d
3575e61
8269f05
9c3a67f
a1edabf
e4a4a8a
5588d18
8445da6
4d12d28
f8d09f8
b33a445
f972f94
4eb549f
b2f87ed
327f6e3
80a28e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,15 @@ import React, { useEffect, Fragment } from 'react'; | |
import { useParams } from 'react-router-dom'; | ||
import { useDispatch, useSelector } from 'react-redux'; | ||
import PropTypes from 'prop-types'; | ||
import { loadEntity, deleteEntity } from '../../store/actions'; | ||
import { loadEntity, deleteEntity, setRosTabVisibility } from '../../store/actions'; | ||
import './InventoryDetail.scss'; | ||
import SystemNotFound from './SystemNotFound'; | ||
import TopBar from './TopBar'; | ||
import FactsInfo from './FactsInfo'; | ||
import { reloadWrapper } from '../../Utilities/index'; | ||
import { addNotification } from '@redhat-cloud-services/frontend-components-notifications/redux'; | ||
import ApplicationDetails from './ApplicationDetails'; | ||
import { getEntitySystemProfile } from '../../api/api'; | ||
import './InventoryDetail.scss'; | ||
|
||
/** | ||
|
@@ -41,7 +42,16 @@ const InventoryDetail = ({ | |
useEffect(() => { | ||
const currId = inventoryId || location.pathname.replace(/\/$/, '').split('/').pop(); | ||
if (!entity || !(entity?.id === currId) || !loaded) { | ||
dispatch(loadEntity(currId, { hasItems: true }, { showTags })); | ||
const action = loadEntity(currId, { hasItems: true }, { showTags }); | ||
dispatch(action); | ||
action.payload.then(() => { | ||
getEntitySystemProfile(currId).then(result => { | ||
const cloudProviderObj = (typeof result.results[0].system_profile.cloud_provider !== 'undefined') | ||
&& result.results[0].system_profile.cloud_provider; | ||
dispatch(setRosTabVisibility((cloudProviderObj === 'aws' || cloudProviderObj === 'azure'))); | ||
return result; | ||
}); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd change the flow of action completely, instead of fetching the data and dispatching specific action for ROS, I'd disable the ROS tab by default. And I'd add new reducer that reacts to Approach in this PR (waiting for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@karelhala Yes by default the tab is disabled even now.
Using the action type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ROS tab should be disabled only on full inventory view. We can wait for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, shouldn't this be handled by the ROS tab itself then? To elaborate. Add an API that allows apps to control the visibility and let them decide to show/hide the content? We would not have to check the content at all. Plus now it's kind of strange. Suddenly, out of nowhere a new tab will appear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's partially handleded by the ROS tab, they currently show empty state. But they don't want to show the tab at all for some users (I was against it, but the PM specifically required it) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I would personally move the responsibility to the team. As I said, provide a component API to specifically state which tabs to show. They will have full control and we have one thing less to worry about. I assume we will get requests for other tabs with similar behavior in the future. I think it may come in handy when you navigate through the URL. This can cause some strange behavior when you navigate directly through the URL, and your tab is missing for some reason and pops up later. EDIT: Furthermore, if they have the empty state logic, they must be calling the APIs as well so we are basically doing the same thing twice for no real reason. Adding the tab visibility as a configuration is even more compelling at this point. |
||
} | ||
}, []); | ||
return <div className="ins-entity-detail"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,3 +65,4 @@ export const CLEAR_FILTERS = 'CLEAR_FILTERS'; | |
export const TOGGLE_TAG_MODAL = 'TOGGLE_TAG_MODAL'; | ||
export const CONFIG_CHANGED = 'CONFIG_CHANGED'; | ||
export const TOGGLE_DRAWER = 'TOGGLE_INVENTORY_DRAWER'; | ||
export const SET_ROS_TAB_VISBILITY = 'SET_ROS_TAB_VISBILITY'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this const |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,8 @@ import { | |
CLEAR_FILTERS, | ||
TOGGLE_TAG_MODAL, | ||
CONFIG_CHANGED, | ||
TOGGLE_DRAWER | ||
TOGGLE_DRAWER, | ||
SET_ROS_TAB_VISBILITY | ||
} from './action-types'; | ||
import { | ||
getEntities as defaultGetEntities, | ||
|
@@ -193,6 +194,11 @@ export const deleteEntity = (systems, displayName) => ({ | |
} | ||
}); | ||
|
||
export const setRosTabVisibility = (shouldShowRosTab) => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need this action, it should be enough to wait for the |
||
type: SET_ROS_TAB_VISBILITY, | ||
payload: shouldShowRosTab | ||
}); | ||
|
||
export const configChanged = (config) => ({ | ||
type: CONFIG_CHANGED, | ||
payload: config | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,11 @@ | ||||||
import { INVENTORY_ACTION_TYPES, ACTION_TYPES, SELECT_ENTITY, SET_INVENTORY_FILTER, SET_PAGINATION } from './action-types'; | ||||||
import { | ||||||
INVENTORY_ACTION_TYPES, | ||||||
ACTION_TYPES, | ||||||
SELECT_ENTITY, | ||||||
SET_INVENTORY_FILTER, | ||||||
SET_PAGINATION, | ||||||
SET_ROS_TAB_VISBILITY | ||||||
} from './action-types'; | ||||||
import systemProfileStore from './systemProfileStore'; | ||||||
import { | ||||||
ComplianceTab, | ||||||
|
@@ -64,6 +71,7 @@ function entityLoaded(state) { | |||||
(!insights.chrome.isProd || (insights.chrome.isProd && insights?.chrome?.isBeta())) && { | ||||||
title: 'Resource Optimization', | ||||||
name: 'ros', | ||||||
isVisible: false, | ||||||
component: RosTab | ||||||
} | ||||||
].filter(Boolean) | ||||||
|
@@ -95,6 +103,17 @@ function entitySelected(state, { payload }) { | |||||
}; | ||||||
} | ||||||
|
||||||
function resourceOptTabVisibility(state, { payload }) { | ||||||
return { | ||||||
...state, | ||||||
activeApps: state.activeApps?.map((entity) => entity.name === 'ros' ? ({ | ||||||
...entity, | ||||||
isVisible: payload | ||||||
}) : entity | ||||||
) | ||||||
}; | ||||||
} | ||||||
|
||||||
function entityDeleted(state, { meta }) { | ||||||
const selected = state.selected || (new Map()); | ||||||
meta.systems.forEach(id => selected.delete(id)); | ||||||
|
@@ -105,11 +124,7 @@ function entityDeleted(state, { meta }) { | |||||
}; | ||||||
} | ||||||
|
||||||
function onEntitiesLoaded(state, { payload, meta }) { | ||||||
if (meta?.lastDateRequest < state?.lastDateRequest) { | ||||||
return state; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rvsia this was added to prevent race conditions, right? I don't think we can remove this. |
||||||
|
||||||
function onEntitiesLoaded(state, { payload }) { | ||||||
return { | ||||||
...state, | ||||||
rows: mergeArraysByKey([state.rows, payload.results.map(result => { | ||||||
|
@@ -160,7 +175,8 @@ export const tableReducer = applyReducerHash( | |||||
|
||||||
export const entitesDetailReducer = () => applyReducerHash( | ||||||
{ | ||||||
[INVENTORY_ACTION_TYPES.LOAD_ENTITY_FULFILLED]: entityLoaded | ||||||
[INVENTORY_ACTION_TYPES.LOAD_ENTITY_FULFILLED]: entityLoaded, | ||||||
[SET_ROS_TAB_VISBILITY]: resourceOptTabVisibility | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use
Suggested change
|
||||||
}, | ||||||
defaultState | ||||||
); | ||||||
|
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 would add another guard in here