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(global-header): added mount points for global header #2168

Conversation

ciiay
Copy link
Contributor

@ciiay ciiay commented Jan 10, 2025

Description

This is for RHIDP-5471

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

  1. In your local rhdh-plugins repo, pull the changes from #270
  2. In /workspaces/global-header/plugins/global-header/ directory, run
npx --yes @janus-idp/cli package export-dynamic-plugin --dynamic-plugins-root ~/<your_path_to_rhdh>/rhdh/dynamic-plugins-root --dev
  1. In your local rhdh repo, add the following code in your app-config.local.yaml file
dynamicPlugins:
  rootDirectory: dynamic-plugins-root
  frontend:
    backstage-plugin-simple-test-components:
      scaffolderFieldExtensions:
        - importName: SimpleTestFieldExtension
    red-hat-developer-hub.backstage-plugin-global-header:
      mountPoints:
        - mountPoint: application/header
          importName: GlobalHeader
          config:
            layout:
              position: above-main-content
  1. In your local rhdh repo, which has changes in this PR, run yarn install and yarn dev

Screen recording:
Locally:

rhidp_5471.mp4

OCP:

rhidp_5471_pr_2168.mp4

Copy link
Contributor

@ciiay ciiay force-pushed the rhidp-5471-create-mountpoints-for-global-header branch from f7700b5 to 5080c3e Compare January 13, 2025 15:22
Copy link
Contributor

@ciiay ciiay force-pushed the rhidp-5471-create-mountpoints-for-global-header branch 2 times, most recently from 964a108 to a123747 Compare January 14, 2025 14:03
Copy link
Contributor

@ciiay
Copy link
Contributor Author

ciiay commented Jan 14, 2025

/test e2e-tests

@ciiay ciiay force-pushed the rhidp-5471-create-mountpoints-for-global-header branch 2 times, most recently from a467323 to 314e8f0 Compare January 14, 2025 18:23
Copy link
Contributor

@ciiay ciiay requested a review from gustavolira January 14, 2025 21:22
Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Some initial feedback:

packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
@ciiay ciiay force-pushed the rhidp-5471-create-mountpoints-for-global-header branch from 9a37dde to ad1eab1 Compare January 14, 2025 22:13
Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

You can just render all "above sidebar" headers and all "above content" headers:

packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Copy link
Contributor

@invincibleJai
Copy link
Contributor

/cc @debsmita1

@openshift-ci openshift-ci bot requested a review from debsmita1 January 22, 2025 14:09
Copy link
Member

@debsmita1 debsmita1 left a comment

Choose a reason for hiding this comment

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

Tested with positions
above-sidebar and above-main-content and invalid position value

/lgtm

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Hi, yep, this looks good! One small suggestion below to extract the new code into a new component.

And then I think this PR should also include some new documentation in https://github.com/redhat-developer/rhdh/blob/main/docs/dynamic-plugins/frontend-plugin-wiring.md

I think a good spot would be after "Customizing and Adding Entity tabs" (before "Provide additional Utility APIs").

packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
packages/app/src/components/Root/Root.tsx Outdated Show resolved Hide resolved
@ciiay ciiay force-pushed the rhidp-5471-create-mountpoints-for-global-header branch from 1db34b3 to 90ec01a Compare January 23, 2025 14:42
ciiay added 2 commits January 23, 2025 11:12
Signed-off-by: Yi Cai <[email protected]>
Copy link
Contributor

Copy link
Contributor

Copy link
Contributor Author

@ciiay ciiay Jan 23, 2025

Choose a reason for hiding this comment

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

Copy link
Member

@christoph-jerolimov christoph-jerolimov left a comment

Choose a reason for hiding this comment

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

Great work @ciiay. The change incl. documentation change lgtm. 👍

We can always improve.

I've tested the container image on a cluster without enabling the plugin. ✔️

With enabled plugin and without any other configuration the header is shown ✔️

global:
  dynamic:
    plugins:
      - package: ./dynamic-plugins/dist/red-hat-developer-hub-backstage-plugin-global-header
        disabled: false

I could also add another header from the test package ✔️

global:
  dynamic:
    plugins:
      - package: ./dynamic-plugins/dist/red-hat-developer-hub-backstage-plugin-global-header
        disabled: false
      - package: '@red-hat-developer-hub/[email protected]'
        integrity: 'sha512-Dlay4DUAC3SifSJx4dmKDeD07DITGX9ZZ2SMCgcMMc00GJVKToD3DFuPYZ7lV2C2Ve7gWDufCf5NUmqaUWf6GA=='
        pluginConfig:
          dynamicPlugins:
            frontend:
              red-hat-developer-hub.backstage-plugin-global-header-test:
                mountPoints:
                  - mountPoint: application/header
                    importName: CrashHeader
                    config:
                      layout:
                        position: above-main-content

above-sidebar works also ✔️

ErrorBoundary works also expected ✔️

/lgtm
/approve

Copy link

openshift-ci bot commented Jan 23, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christoph-jerolimov, debsmita1

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:
  • OWNERS [christoph-jerolimov,debsmita1]

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

@christoph-jerolimov
Copy link
Member

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit 9a7353c into redhat-developer:main Jan 24, 2025
11 checks passed
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.

5 participants