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

Overhaul network layout #1035

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

edlerd
Copy link
Collaborator

@edlerd edlerd commented Dec 17, 2024

Done

  • overhaul network layout
  • add network topology map for network detail pages
  • add network config key search
  • combine network overview and configuration tabs into one

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • explore network creation and editing

@webteam-app
Copy link

Copy link
Collaborator

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

Love the network topology and how the existing form is reused. Still need to go through the css files though.

src/pages/networks/NetworkList.tsx Outdated Show resolved Hide resolved
src/pages/networks/EditNetwork.tsx Show resolved Hide resolved
src/pages/networks/CreateNetwork.tsx Show resolved Hide resolved
src/pages/networks/NetworkTopology.tsx Show resolved Hide resolved
src/pages/networks/NetworkTopology.tsx Show resolved Hide resolved
src/pages/networks/NetworkTopology.tsx Outdated Show resolved Hide resolved
src/pages/networks/NetworkTopology.tsx Show resolved Hide resolved
src/pages/networks/forms/NetworkForm.tsx Show resolved Hide resolved
src/pages/networks/forms/NetworkForm.tsx Show resolved Hide resolved
src/pages/networks/forms/NetworkFormMain.tsx Outdated Show resolved Hide resolved
@mas-who
Copy link
Collaborator

mas-who commented Jan 7, 2025

Please see some QA observations below:

  1. Can we align the text for the network detail page general section? There is probably some margin that needs to be removed.
    Screenshot from 2025-01-07 12-35-27

  2. On smaller screens, on the create network page (edit page is fine), the config nav with the search bar seems to overlap with other page contents. Also maybe the nav can become collapsible on small screens?
    Screenshot from 2025-01-07 12-55-03

  3. When creating a network, if name is not entered we can switch to the yaml editor. Seems a little strange since we can set the network name in the yaml editor itself. Perhaps this is something to think about in the long run as it affects other creation forms too. Also we would have to figure out how to handle name field population in the yaml editor etc...
    Screenshot from 2025-01-07 13-28-43

  4. Should we show the downstream instances connected to the ovn networks in the following scenario? Probably a design question for @piperdeck . If not, we should probably get rid of the unconnected lines at the end of the topology.
    Note that in this scenario, I have instance i1 connected to the uplink as well as several ovn networks, the topology presented makes me think that i1 is only connected to the uplink network. wdyt?
    Screenshot from 2025-01-07 13-46-53

@edlerd edlerd force-pushed the network-overhaul branch 3 times, most recently from d8201a1 to de21bfe Compare January 7, 2025 15:20
@edlerd
Copy link
Collaborator Author

edlerd commented Jan 7, 2025

Please see some QA observations below:

  1. Can we align the text for the network detail page general section? There is probably some margin that needs to be removed.

Yes, good point, fixed the alignment.

  1. On smaller screens, on the create network page (edit page is fine), the config nav with the search bar seems to overlap with other page contents. Also maybe the nav can become collapsible on small screens?

I think we should hide the nav and search bar on small screens, which is done with recent changes.

  1. When creating a network, if name is not entered we can switch to the yaml editor. Seems a little strange since we can set the network name in the yaml editor itself. Perhaps this is something to think about in the long run as it affects other creation forms too. Also we would have to figure out how to handle name field population in the yaml editor etc...

I think the name is mandatory to switch to the yaml editor, and that should be fine in my opinion. The editor would only be used for some advanced configuration that is not supported in the ui.

  1. Should we show the downstream instances connected to the ovn networks in the following scenario? Probably a design question for @piperdeck . If not, we should probably get rid of the unconnected lines at the end of the topology.

I am now adding a line and three dots to downstream networks that have descendants. I discussed this with @piperdeck a while back, and I think we both agreed to only focus on direct neighbours in the topology and announcing further connections in this way. So we keep the focus on the current network and users can easily jump to the next network to explore further.

@piperdeck
Copy link

@edlerd

I've left some comments and feedback in a Figma page: https://www.figma.com/design/obSXJ4VkJUa2fHjt5lNf68/25.04-OVN-Networking?node-id=242-9623

Copy link
Collaborator

@mas-who mas-who left a comment

Choose a reason for hiding this comment

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

It's working very nicely, just a few more observations.

src/pages/networks/NetworkTopology.tsx Show resolved Hide resolved
src/pages/networks/forms/NetworkForm.tsx Show resolved Hide resolved
src/pages/networks/EditNetwork.tsx Show resolved Hide resolved
src/sass/_ip_address.scss Show resolved Hide resolved
src/sass/_forms.scss Outdated Show resolved Hide resolved
src/sass/_network_topology.scss Show resolved Hide resolved
@edlerd edlerd requested a review from mas-who January 8, 2025 12:20
@edlerd
Copy link
Collaborator Author

edlerd commented Jan 8, 2025

Thanks for the comments and suggestions @piperdeck @mas-who and @lyubomir-popov
Please have another look, with the recent commit all things should be addressed.

@mas-who
Copy link
Collaborator

mas-who commented Jan 8, 2025

LGTM from my end 👍

Kxiru and others added 8 commits January 9, 2025 16:51
Signed-off-by: Nkeiruka <[email protected]>
Signed-off-by: David Edler <[email protected]>
 - use new view for managed networks, remove old network overview
 - topology should be calculated for managed networks
 - search placeholder: "Search for key"
 - search over the description and field name
 - hide topology on yaml config
 - on creation: asterisk for name and uplink/parent
 - on creation: disable submit until a parent is selected
 - left align topology

Signed-off-by: David Edler <[email protected]>
@edlerd edlerd force-pushed the network-overhaul branch 2 times, most recently from 22bcff6 to ee27b49 Compare January 9, 2025 15:53
@edlerd
Copy link
Collaborator Author

edlerd commented Jan 11, 2025

@piperdeck gave design +1 yesterday. @mas-who if you have some time on Monday, please give another review of the updates.

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.

6 participants