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): initial PR for global header plugin #203

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

Conversation

ciiay
Copy link
Contributor

@ciiay ciiay commented Dec 16, 2024

Hey, I just made a Pull Request!

Initial PR for RHIDP-5251

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Screen recordings

plugins/global-header test
rhidp_5251.mp4

dev env test

rhidp_5251_dev.mp4

@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Dec 16, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
app workspaces/global-header/packages/app none v0.0.0
backend workspaces/global-header/packages/backend none v0.0.0
@red-hat-developer-hub/backstage-plugin-global-header workspaces/global-header/plugins/global-header none v0.0.0

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.

Thanks, a great initial PR. 👍

Some small comments below and I think it's worthful if you can also address here:

  1. Add a line to .github/CODEOWNERs :)
  2. Please add a backstage app. I think its in general a good way to test plugins, but it's esp. useful for this plugin.
  3. Address my comments below and (optional if you really want do that in a follow-up PR, but I think there no reason to rush ;)) split the GlobalHeader file a bit and remove the examples components.

@ciiay ciiay force-pushed the rhidp-5251-create-global-header-plugin branch from 65fbdaa to 81e7ead Compare December 18, 2024 02:17
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.

Would be great if you can take a look into the comments below.

Here or in a follow up PR is up to you. But since you should also check why the build is failing, it's maybe easier to address them here directly.

Everything else looks nice!

.github/CODEOWNERS Outdated Show resolved Hide resolved
workspaces/global-header/app-config.yaml Outdated Show resolved Hide resolved
workspaces/global-header/app-config.yaml Show resolved Hide resolved
workspaces/global-header/.yarnrc.yml Outdated Show resolved Hide resolved
@christoph-jerolimov
Copy link
Member

@ciiay for the typescript errors in node_modules (duplicate type or something) we changes some of the workspaces to skipLibCheck (that means it doesn't validate node_module TS definitions):

➜ ~/git/redhat-developer/rhdh-plugins (main) ✔ grep skipLibCheck workspaces/*/package.json

workspaces/bulk-import/package.json:    "tsc:full": "tsc --skipLibCheck true --incremental false",
workspaces/homepage/package.json:    "tsc:full": "tsc --skipLibCheck false --incremental false",
workspaces/lightspeed/package.json:    "tsc:full": "tsc --skipLibCheck true --incremental false",
workspaces/marketplace/package.json:    "tsc:full": "tsc --skipLibCheck false --incremental false",
workspaces/openshift-image-registry/package.json:    "tsc:full": "tsc --skipLibCheck true --incremental false",
workspaces/orchestrator/package.json:    "tsc:full": "tsc --skipLibCheck true --incremental false",
workspaces/repo-tools/package.json:    "tsc:full": "tsc --skipLibCheck false --incremental false",
workspaces/theme/package.json:    "tsc:full": "tsc --skipLibCheck false --incremental false",

It's somehow related to a Node.js type and and some library type mismatches. It's fine to disable that check here as well.

@ciiay ciiay force-pushed the rhidp-5251-create-global-header-plugin branch from 8e51028 to bdfe5b3 Compare December 20, 2024 04:52
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Signed-off-by: Yi Cai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants