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

feat(pci.private-network): Listing page - Listing page revamp #14210

Open
wants to merge 10 commits into
base: feat/private-network-refactor1
Choose a base branch
from

Conversation

Tsiorifamonjena
Copy link
Contributor

@Tsiorifamonjena Tsiorifamonjena commented Nov 21, 2024

ref: TAPC-1826

Question Answer
Branch? feat/private-network-refactor1
Bug fix? no
New feature? no
Breaking change? no
Tickets Fix #TAPC-1826
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

Comment on lines 174 to 182
{dhcpEnabled
? renderChip(
t('pci_projects_project_network_private_dhcp_active'),
ODS_THEME_COLOR_INTENT.success,
)
: renderChip(
t('pci_projects_project_network_private_dhcp_disabled'),
ODS_THEME_COLOR_INTENT.warning,
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why create the rendChip and not use react component ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not outdated, still an issue

Comment on lines 12 to 15
const color = {
[ResourceStatus.ACTIVE]: ODS_THEME_COLOR_INTENT.success,
[ResourceStatus.DISABLED]: ODS_THEME_COLOR_INTENT.warning,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be outside of component


return (
<OsdsTabPanel
active={activeTab === PrivateNetworkTabName.LOCAL_ZONE_TAB_NAME}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used ? If the component is displayed then we are on the tab

name={PrivateNetworkTabName.LOCAL_ZONE_TAB_NAME}
>
<Notifications />
{isPending ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should reconsider this since there is a lot of requests running in parallel we could display data network by network and display a spinner above.

This could allow to have one region down while we can still access the page

Comment on lines -48 to 51
path: ROUTE_PATHS.delete,
path: '',
...lazyRouteConfig(() =>
import('@/pages/delete/DeleteNetwork.page'),
import('@/pages/listing/region/PrivateNetworkRegion.page'),
),
handle: {
tracking: 'delete',
},
},
],
},
{
path: ROUTE_PATHS.localZone,
handle: {
tracking: 'localZone',
},
...lazyRouteConfig(() => import('@/pages/list/List.page')),
children: [
{
path: ROUTE_PATHS.delete,
path: ROUTE_PATHS.localZone,
...lazyRouteConfig(() =>
import('@/pages/delete/DeleteNetwork.page'),
import('@/pages/listing/localZone/PrivateNetworkLZ.page'),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Routing seems weird, I think we should use query params instead but I don't understand how LZ is different from classic regions

@Tsiorifamonjena Tsiorifamonjena force-pushed the feat/pci-private-network_tapc-1826 branch from 8b09ba6 to c865939 Compare November 29, 2024 17:50
Copy link

sonarcloud bot commented Nov 29, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants