NavSection: allow plugins to define their position

[mareklibra]

Within NavSection, a plugin can provide mergeAfter optional string property
to denote after which element will be rendered.

If mergeAfter is omitted or not-matching, the item is appended at the end.

First use-case: Virtual Machines of the kubevirt-plugin will be positioned after Pods as requested by UXD.

[openshift-ci-robot]

Hi @mareklibra. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vojtechszocs]

The "merge after" behavior is related to plugin-contributed nav links only, we shouldn't bloat existing nav link components themselves.

 interface NavItem {
   section: NavSectionTitle;
   componentProps: Pick<NavLinkProps, 'name' | 'required' | 'disallowed' | 'startsWith'>;
+  mergeAfter?: string;
}

[mareklibra]

done

[vojtechszocs]

This shouldn't be necessary, Console NavList has no use for this prop.

[mareklibra]

ok, done

[vojtechszocs]

Nit-pick: space before pc.

[mareklibra]

done, reworked due to change above

[vojtechszocs]

Nit-pick: the pc part of condition shouldn't be necessary (?)

[mareklibra]

formerly the pc could be null - the _.compact() was farther modified by map().
But reworked due to change above.

[vojtechszocs]

Note that React.Children.map may return null or undefined if there are no children.

Consequently, mergeElements function should be prepared to handle this situation.

[mareklibra]

done

[spadgett]

Better to add types to function declarations

[vojtechszocs]

Do we really need all those parameters?

mapChild function has this bound through class field arrow function syntax, giving us access to this.props and this.state like in other methods.

const { activeNamespace, flags } = this.props;
const { activeChild } = this.state;

As for the c parameter, it's currently inferred by TS as any because we're calling React.Children.map without specifying <T, C> type parameters (this code came from .jsx file).

For now, I'd suggest typing c as c: React.ReactElement (without type parameters) since the method needs access to c.props.

[mareklibra]

Types added.

I considered the function to be single-purposed, just extracted from former render() closure to improve readability.

As requested, I changed it to access this directly but moved inside class definition as it expects this to meet certain contract.

[spadgett]

/assign @vojtechszocs
/approve

I haven't reviewed this closely, but approving the general change. @vojtechszocs feel free to add your slash-lgtm when this is ready.

[spadgett]

/ok-to-test

[vojtechszocs]

Do we really need all those parameters?

mapChild function has this bound through class field arrow function syntax, giving us access to this.props and this.state like in other methods.

const { activeNamespace, flags } = this.props;
const { activeChild } = this.state;

As for the c parameter, it's currently inferred by TS as any because we're calling React.Children.map without specifying <T, C> type parameters (this code came from .jsx file).

For now, I'd suggest typing c as c: React.ReactElement (without type parameters) since the method needs access to c.props.

[vojtechszocs]

Please add proper typings:

const mergePluginChild = (Children: React.ReactElement[], pluginChild: React.ReactElement, mergeAfter?: string) => {};

[mareklibra]

done

[vojtechszocs]

Code complexity can be reduced:

const index = mergeAfter ? Children.findIndex(c => c.props.name === mergeAfter) : -1;
if (index >= 0) {
  Children.splice(index + 1, 0, pluginChild);
} else {
  Children.push(pluginChild);
}

[mareklibra]

done

[vojtechszocs]

Please add proper typings and use a method name that reflects the fact we're returning a React component created from the given nav item:

const getNavLink = (item: plugins.NavItem, key: number): NavLink<any> | null => {};

You'll probably need to import NavLink type from ./items.

[mareklibra]

done with React.ReactElement<NavLink<any>>

[vojtechszocs]

With mapChild having a single c argument, this helper function isn't needed.

[mareklibra]

Yes, removed.
Anyway, .bind(this) needs to be used within React.Children.map() bellow.

[vojtechszocs]

@mareklibra @spadgett LGTM, posted some nit-picks regarding redundant type annotations (aiming for concise code).

[vojtechszocs]

Nit-pick: the return type declaration React.ReactElement | null can be removed since TS compiler can infer that from code analysis.

This is similar to writing const foo = true instead of const foo: boolean = true, which keeps the code shorter.

[vojtechszocs]

Nit-pick: the React.ReactElement | null return type can be automatically inferred, I'd remove it.

[vojtechszocs]

Nit-pick: the type of Children can be automatically inferred, I'd remove it.

[vojtechszocs]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, spadgett, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment