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

add extension to contribute arbitrary pages #1668

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Jun 4, 2019

Adds extension to allow plugins to contribute pages which render arbitrary react components. This is required by the dev console to render pages that are not based on a single resource and therefore cannot be contributed through the existing resource list / details page extensions.

Changes

  • Updated demo plugin to contribute new page linked to nav item
  • renamed resourceDetailPages to resourceDetailsPages
  • Updated typescript types for resource details / list page extensions to align with their counterpart in core
  • Renamed type of existing extensions ResourcePage/List and ResourcePage/Detail
    • This would be a breaking change for existing PRs
    • I wanted to align all page extensions to use the same prefix. Please comment.

/cc @vojtechszocs @spadgett
fyi @jtomasek @rohitkrai03

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 4, 2019
@spadgett spadgett added this to the v4.2 milestone Jun 4, 2019
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 4, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2019
@rohitkrai03
Copy link
Contributor

@christianvogt How do we handle shared routes for use cases where we want two different perspectives to share the same view using this plugin? Is there a way for the plugin to say that I want to reuse a particular view from main console app but in a different perspective?

@christianvogt christianvogt force-pushed the ext-pages branch 2 times, most recently from e57e9a2 to 1e277dc Compare June 4, 2019 20:21
@christianvogt
Copy link
Contributor Author

@rohitkrai03 It was decided a while back that the dev perspective is just a customized nav. So all views are available to all perspectives.

@christianvogt
Copy link
Contributor Author

/retest

@christianvogt
Copy link
Contributor Author

/retest

@@ -90,7 +92,7 @@ const plugin: Plugin<ConsumedExtensions> = [
},
},
{
type: 'ResourcePage/Detail',
type: 'Page/Resource/Details',
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Why plural, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several references in code to both detail and details (more plural); with no apparent consistency. It seemed to me the choice for plural made more sense as per the contents of the pages containing several sections under a tabbed navigation. This to me implied several details as oppose to a single all inclusive detail about the resource.

If there is a particular reason to use the singular form, I'm happy to go with that as I am unfamiliar with the history of this repo.

@spadgett thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Like @christianvogt, I have a preference for "details." It's not a single detail. Resources have many properties. It's many details :)

There's no particular history that I'm aware of. We should try to be consistent.

@jtomasek
Copy link

jtomasek commented Jun 5, 2019

/retest

@spadgett
Copy link
Member

spadgett commented Jun 5, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. queue/blocks-others size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants