-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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: Add customization support for UI components #4634
feat: Add customization support for UI components #4634
Conversation
✅ Deploy Preview for ohif-dev canceled.
|
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
we are reworking the customizationService so stay tuned until it is done |
Thank you for the information, @sedghi. Would you happen to have a feature branch or work item that could serve as a reference for the implementation? |
247b7a3
to
8e39169
Compare
Can you rebase on top of the new customization service and follow what's done there please? Sorry for the inconvenience |
d9d5415
to
79b8f42
Compare
@sedghi I have updated the branch based on the latest changes in the customization service. Would you mind reviewing it when you have a moment? |
extensions/default/src/customizations/contextMenuItemCustomization.ts
Outdated
Show resolved
Hide resolved
extensions/default/src/customizations/labellingFlowCustomization.tsx
Outdated
Show resolved
Hide resolved
platform/docs/versioned_docs/version-3.9/platform/services/ui/customization-service.md
Outdated
Show resolved
Hide resolved
platform/ui/src/components/ProgressLoadingBar/ProgressLoadingBar.tsx
Outdated
Show resolved
Hide resolved
platform/ui/src/components/LoadingIndicatorTotalPercent/LoadingIndicatorTotalPercent.tsx
Outdated
Show resolved
Hide resolved
platform/ui/src/components/LoadingIndicatorProgress/LoadingIndicatorProgress.tsx
Outdated
Show resolved
Hide resolved
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.
See my comments thanks
platform/ui/src/components/LoadingIndicatorProgress/LoadingIndicatorProgress.tsx
Outdated
Show resolved
Hide resolved
platform/ui/src/components/LoadingIndicatorTotalPercent/LoadingIndicatorTotalPercent.tsx
Outdated
Show resolved
Hide resolved
platform/ui/src/components/ProgressLoadingBar/ProgressLoadingBar.tsx
Outdated
Show resolved
Hide resolved
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.
I really appreciate the PR, which is pretty much in line with our goal of making each component customizable.
I'm not entirely sold on the CustomizableRenderComponent
idea, though. You're basically replacing the entire component logic with a customized version, and I'm not sure that's the right approach. This could have some pretty significant side effects - for instance, what if certain parts of the app want to use the default version, while others want to use a customized ContextMenu?
I think a better way to handle this is to consider customization as an all-or-nothing proposition outside of the platform/ui and platform/ui-next. I'm not a fan of calling the customization service directly from our platform/ui and ui-next, as that means services logic is leaking into the UI. I know some components are already doing this, but I'm in the process of cleaning them up. In my opinion, there shouldn't be any mention of services, commands, or managers in the platform/ui and ui-next.
I'd rather see more instances of customizationService.getCustomization('ui.contextMenuItem')
than injecting the customization service at the component level. That way, we can keep the customization logic separate from the UI components. Does that make sense?
CCing @wayfarer3130 for his opinion
… based on latest customization service
1dbee9a
to
5f4dec2
Compare
@sedghi I have executed the tests after the Bun installation, and everything is working perfectly. |
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.
Thanks a lot, this is great
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment