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.

Sign up for GitHub

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

Jump to bottom

feat(xo-6): add pool network view #8182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(xo-6): add pool network view #8182

wants to merge 1 commit into from

Conversation

S3bastianCZ
Copy link
Contributor

Description

Add Pool network view with side panel

SCREENSHOT

Capture d’écran 2024-12-09 à 16 09 20

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

  • develop skills of junior reviewers
  • limit the workload for senior reviewers
  • limit the number of unnecessary changes by the author
  1. The author creates a PR.
  2. Review process:
    1. The author assigns the junior reviewer.
    2. The junior reviewer conducts their review:
      • Resolves their comments if they are addressed.
      • Adds comments if necessary or approves the PR.
    3. The junior reviewer assigns the senior reviewer.
    4. The senior reviewer conducts their review:
      • If there are no unresolved comments on the PR → merge.
      • Otherwise, we continue with 3.
  3. The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

  1. The author can request a review at any time, even if the PR is still a Draft.
  2. In theory, there should not be more than one reviewer at a time.
  3. The author should not make any changes:
    • When a reviewer is assigned.
    • Between the junior and senior reviews.

@S3bastianCZ S3bastianCZ self-assigned this Dec 9, 2024
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch 2 times, most recently from dce89a5 to 4c38fd5 Compare December 10, 2024 17:36
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch 2 times, most recently from 825d888 to dd74a4b Compare December 11, 2024 12:56
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch 2 times, most recently from 0d2d1a0 to a17af42 Compare January 6, 2025 12:44
@S3bastianCZ S3bastianCZ marked this pull request as ready for review January 6, 2025 12:57
OlivierFL
OlivierFL requested changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@OlivierFL OlivierFL left a comment

Choose a reason for hiding this comment

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

There are other big changes to do on this PR, but to avoid making a lot of complicated comments, I will make another PR with the refactored code. I will link it when ready.

@xen-orchestra/lite/src/locales/en.json Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you restore the blank lines in the locales’ files?

@xen-orchestra/web/src/components/pool/PoolHostInternalNetworkTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolHostInternalNetworkTable.vue Outdated
Comment on lines 211 to 201
define('checkbox', () => '', { label: '', isHideable: false }),
define('name_label', { label: 'Name', isHideable: true }),
define('name_description', { label: 'Description', isHideable: true }),
define('MTU', { label: 'MTU', isHideable: true }),
define('defaultIsLocked', { label: 'Default Locking mode', isHideable: true }),
define('more', () => '', { label: '', isHideable: false }),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since isHideable is true by default, there is no need to set it explicitly.
Also, remember to translate all strings for the labels.

Suggested change
define('checkbox', () => '', { label: '', isHideable: false }),
define('name_label', { label: 'Name', isHideable: true }),
define('name_description', { label: 'Description', isHideable: true }),
define('MTU', { label: 'MTU', isHideable: true }),
define('defaultIsLocked', { label: 'Default Locking mode', isHideable: true }),
define('more', () => '', { label: '', isHideable: false }),
define('checkbox', () => '', { label: '', isHideable: false }),
define('name_label', { label: t('name') }),
define('name_description', { label: t('description') }),
define('MTU', { label: t('mtu') }),
define('defaultIsLocked', { label: t('default-locking-mode') }),
define('more', () => '', { label: '', isHideable: false }),

It's just an example, be sure to add the correct keys in the locales.

@xen-orchestra/web/src/components/pool/PoolHostInternalNetworkTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolNetworksTable.vue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be sure to translate all strings in this file (see my comments on @xen-orchestra/web/src/components/pool/PoolHostInternalNetworkTable.vue)

@xen-orchestra/web/src/components/pool/PoolNetworksTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/utils/xo-api-definition.util.ts Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolNetworksTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolNetworksSidePanel.vue Outdated
Comment on lines 130 to 144
const getHost = (id: XoPif['$host']) => {
return records.value.find(host => host.id === id)
}

const getNetworkVlan = (vlan: XoPif['vlan']) => {
return vlan !== -1 ? vlan.toString() : t('none')
}

const getLockingMode = (network: XoNetwork) => {
return network.defaultIsLocked ? t('disabled') : t('unlocked')
}

const getNbd = (network: XoNetwork) => {
return network.nbd ? t('on') : t('off')
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not a good practice to use functions called from the template. Here, computed would be more appropriate.

@xen-orchestra/web/src/components/pool/PoolNetworksSidePanel.vue Outdated Show resolved Hide resolved
@OlivierFL OlivierFL requested a review from ByScripts January 15, 2025 09:39
@S3bastianCZ S3bastianCZ requested a review from OlivierFL January 15, 2025 15:19
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch from e02c67f to bf042a2 Compare January 16, 2025 09:23
OlivierFL
OlivierFL requested changes Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@OlivierFL OlivierFL left a comment

Choose a reason for hiding this comment

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

There are other changes that need to be made, here is the link to my PR: https://github.com/vatesfr/xen-orchestra/pull/8252/files

@xen-orchestra/web-core/lib/locales/fr.json Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolHostInternalNetworkTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/components/pool/PoolNetworksTable.vue Outdated Show resolved Hide resolved
@xen-orchestra/web/src/stores/xo-rest-api/network.store.ts Outdated Show resolved Hide resolved
@xen-orchestra/web/src/stores/xo-rest-api/network.store.ts Outdated Show resolved Hide resolved
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch 7 times, most recently from 87b50f1 to 8ce3eba Compare January 22, 2025 14:38
@S3bastianCZ S3bastianCZ force-pushed the xo-6/pool-netowrk-view branch from 8ce3eba to bcfb604 Compare January 22, 2025 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlivierFL OlivierFL OlivierFL requested changes

@ByScripts ByScripts Awaiting requested review from ByScripts

Requested changes must be addressed to merge this pull request.

Assignees

@S3bastianCZ S3bastianCZ

Labels
XO Web 6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@S3bastianCZ @OlivierFL @CzechSebastian