Allow UI elements to be injected and remove Antd dependency #520
Conversation
|
This is just a draft to get feedback on this approach. |
aocenas
force-pushed
the
lift-antd-dependencies
branch
from
26c3fdc
to
f2e3a19
Compare
Signed-off-by: Andrej Ocenas <mr.ocenas@gmail.com>
aocenas
force-pushed
the
lift-antd-dependencies
branch
from
f2e3a19
to
ba8f09b
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #520 +/- ##
==========================================
+ Coverage 92.92% 96.57% +3.64%
==========================================
Files 197 254 +57
Lines 4808 7620 +2812
Branches 1160 1986 +826
==========================================
+ Hits 4468 7359 +2891
+ Misses 299 261 -38
+ Partials 41 0 -41 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Minor changes (variable name, use a const, a type, and possibly avoiding manually checking for undefined), but otherwise looks like a good start.
Though the title is a bit confusing as it does not remove antd as a dependency, it just removes it as a dependency of TracePage components.
| placement?: TooltipPlacement; | ||
| children?: React.ReactNode; | ||
| }, | ||
| {} |
| updateViewRangeTime={this.updateViewRangeTime} | ||
| viewRange={viewRange} | ||
| /> | ||
| <UIElementsContext.Provider value={{ Popover }}> |
There was a problem hiding this comment.
The value here should be a const at the top of the file. Something like const UI_ELEMENTS = { Popover };. Without that, ever render of TracePage/index will cause all consumers to re-render (source).
| export function GetElementsContext(props: GetElementsContextProps) { | ||
| return ( | ||
| <UIElementsContext.Consumer> | ||
| {(value: Elements | undefined) => { |
There was a problem hiding this comment.
Similar to my feedback on another PR, is it possible to remove the | undefined and rely on TypeScript to check that the value is provided?
There was a problem hiding this comment.
Although I suppose that depends on a way to check that UiElementsContext.Consumer is always used within a UiElementsContext.Provider 🤔
| */ | ||
| export function GetElementsContext(props: GetElementsContextProps) { | ||
| return ( | ||
| <UIElementsContext.Consumer> |
There was a problem hiding this comment.
UI should be Ui. For the sake of camel/pascal case, acronyms should be treated as a word. UiFind as an example
|
Also, how will this impact our ability to add functionality to JaegerUI? For instance, the |
Which problem is this PR solving?
Part of both #509 and #508
Short description of the changes
This tries to remove dependency on Antd. Issue with Antd is first the size of the overall dependency, global styles and problematic theming that would have theme both the trace UI and the Antd elements separately.
This provides a react context that needs to be used for injecting required UI components. This would allow injecting custom UI element to make theming better. For example in Grafana we use custom component library to share styling and behaviour.
Caveats: