-
Notifications
You must be signed in to change notification settings - Fork 157
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(fab): add new mountpoints support for providers and listeners #2199
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cf55126
to
497eb35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Tested locally and it seems that it works functionally, only with a few UI issues. Check out the screen recording:
pr_2199_review.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I know it's WIP. The first version looks good!
Two suggestion I have: I think we should move the integration part into AppBase and create new components for Listener and Provider.
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
Similar to #2168: I I think a good spot would be after "Customizing and Adding Entity tabs" (before "Provide additional Utility APIs"). Merge conflicts, here we go. I'm fine if you want do that in a 2nd PR, but IMO it should be part of that story. :)
91c69f3
to
d61135e
Compare
The image is available at: |
d61135e
to
e6129c4
Compare
e6129c4
to
011d027
Compare
The image is available at: |
011d027
to
b833dde
Compare
The image is available at: |
b833dde
to
080016d
Compare
The image is available at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, great so far, but I have added some comments to simplify the documentation.
It looks to me that the Provider and Listener are currently the same thing, just somewhere else.
What would help for both (esp. for the provider to see the issue) is that you add some unit tests for both.
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intention could be improved
...ins/wrappers/red-hat-developer-hub-backstage-plugin-global-floating-action-button/turbo.json
Show resolved
Hide resolved
080016d
to
e7a41b7
Compare
The image is available at: |
@debsmita1: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
e7a41b7
to
e79acd7
Compare
e79acd7
to
51623c8
Compare
The image is available at: |
Description:
https://issues.redhat.com/browse/RHIDP-5478
https://issues.redhat.com/browse/RHIDP-5399
How to test changes / Special notes to the reviewer:
Testing in your local
rhdh
repo, add the following snippet in yourapp-config.yaml
filerhdh
repo, runyarn install
followed byyarn dev
Testing on a cluster
Screenshot/GIF
Screen.Recording.2025-01-30.at.4.34.46.PM.mov
PR acceptance criteria
Please make sure that the following steps are complete: