[FEATURE] UI Customization: Component Provider - #241
Conversation
3701918 to
88d7b67
Compare
88d7b67 to
6f911e9
Compare
6f911e9 to
d2bc721
Compare
3db4542 to
b63342b
Compare
| } | ||
|
|
||
| /* ---- Icon ---- */ | ||
| .ps-Alert-icon { |
There was a problem hiding this comment.
icon should be its own component and we should compose it into the alert, rather than defining specific styling in alerts, this would make the Icon reusable in other contexts
There was a problem hiding this comment.
If custom styling is needed for a composed component, then we can use the component class, e.g.:
.ps-Alert > .ps-Icon { ... }There was a problem hiding this comment.
Separated Icon as its own reusable component and updated the Alert files.
| <div role={role} {...rest} ref={ref} className={classes} data-severity={severity}> | ||
| {children} | ||
| {resolvedIcon !== null && ( | ||
| <span className="ps-Alert-icon" aria-hidden="true"> |
There was a problem hiding this comment.
We should use BEM conventions here https://getbem.com/naming/
There was a problem hiding this comment.
Updated to ps-Alert__icon
| cursor: wait; | ||
| } | ||
|
|
||
| .ps-Button-spinner { |
There was a problem hiding this comment.
Spinner should be also its own primitive component and we should compose it into the button when loading
There was a problem hiding this comment.
Separated Spinner as its own primitive component.
There was a problem hiding this comment.
We should group the context providers and context into a contexts folder
There was a problem hiding this comment.
Grouped into contexts
| } | ||
|
|
||
| export interface PersesIcons { | ||
| Error: React.ComponentType<React.SVGProps<SVGSVGElement>>; |
There was a problem hiding this comment.
| Error: React.ComponentType<React.SVGProps<SVGSVGElement>>; | |
| Error: React.ComponentType<SVGProps<SVGSVGElement>>; |
| import type { ButtonProps } from './primitives/Button/Button'; | ||
|
|
||
| export interface PersesComponents { | ||
| Button: React.ComponentType<ButtonProps>; |
There was a problem hiding this comment.
| Button: React.ComponentType<ButtonProps>; | |
| Button: ComponentType<ButtonProps>; |
| ComponentsContextValue, | ||
| } from './ComponentsProvider'; | ||
|
|
||
| export { ErrorIcon, InfoIcon, SuccessIcon, WarningIcon } from './icons'; |
There was a problem hiding this comment.
We should avoid exporting all icons and components from this file, otherwise we will force consumers to load all the components even if they are not used. A better approach would be to expose a file they can import with the list of all the default components and icons, and initialize the application with the default ones or custom ones.
There was a problem hiding this comment.
Updated ComponentsProvider no longer imports Button/Alert/icons internally
There was a problem hiding this comment.
This file still exporting all primitives and icons, if a consumer imports @perses-dev/components/next they will be forced to import all components, even if they don't need them. We should export only the required components so the icons and components can be configured not the components and icons themselves.
|
|
||
| export type { PersesComponents, PersesIcons, ComponentsContextValue, ComponentsProviderProps }; | ||
|
|
||
| const DEFAULT_COMPONENTS: PersesComponents = { |
There was a problem hiding this comment.
Instead of merging the default components we should make the components and icons in the provider mandatory, meaning consumers must initialize the components, even if they want to use the defaults. This will allow consumers to load only what they need without bloating their bundles with unnecessary components.
There was a problem hiding this comment.
Updated ComponentsProvider so that the consumer must initialize default components.
| } | ||
|
|
||
| export function mergeRefs<T>( | ||
| innerRef: React.MutableRefObject<T | null>, |
There was a problem hiding this comment.
| innerRef: React.MutableRefObject<T | null>, | |
| innerRef: MutableRefObject<T | null>, |
Signed-off-by: Jenny Zhu <jenny.a.zhu@gmail.com>
25dfa3d to
e445932
Compare
| const isDark = globalState.theme === 'dark'; | ||
|
|
||
| useEffect(() => { | ||
| document.documentElement.setAttribute('data-perses-mode', isDark ? 'dark' : 'light'); |
There was a problem hiding this comment.
I think it would be better to create a ThemeModeProvider provider and include it here rather than doing this manually. The css does not need to be inlined as we can create a css file and import it.
|
|
||
| let resolvedIcon: ReactNode; | ||
|
|
||
| if (icon !== undefined) { |
There was a problem hiding this comment.
Icon might be false or 0 and this will render an empty icon
| const DEFAULT_SEVERITY_ICONS: Record<AlertSeverity, ComponentType<SVGProps<SVGSVGElement>>> = { | ||
| success: SuccessIcon, | ||
| info: InfoIcon, | ||
| warning: WarningIcon, | ||
| error: ErrorIcon, | ||
| }; | ||
|
|
||
| const SEVERITY_TO_PROVIDER_KEY: Record<AlertSeverity, keyof PersesIcons> = { | ||
| error: 'Error', | ||
| warning: 'Warning', | ||
| success: 'Success', | ||
| info: 'Info', | ||
| }; |
There was a problem hiding this comment.
This 2 mappings can be merged into one with a key and icon fields, so we don't have to maintain them synchronized
| hoverBg: string; | ||
| } | ||
|
|
||
| const PF_SOLID: Record<string, PfPalette> = { |
There was a problem hiding this comment.
Not sure this is the right way to test this. At this point we are aiming to test that the ComponentProvider can receive custom components to override the default one. Not something specific to Patternfly. We also need to test that overriding the tokens used by the component produces the intended customization. I suggest to simplify this custom Button, remove the Patternfly specifics and add a new story with customized tokens.
| .env.production.local | ||
| webpack.local.js | ||
|
|
||
| .npmrc |
Description
Create Component Provider as continuation of this initiative: UI Customization (perses/perses#3570).
Previous PRs towards this initiative:
Below is the Ladel story showing usage of the Component Provider to style Button and Alert components in line with Patternfly v6:
Screenshots
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
See e2e docs for more details. Common issues include: