chore(docs): refactor places-ui-kit example into dedicated components for UI Kit elements #883
Conversation
pappkartoni
changed the title
usefulthink
left a comment
usefulthink
left a comment
There was a problem hiding this comment.
Awesome, thanks. Thats exactly the style of API I would imagine for the places ui kit components. A couple of smaller remarks in the code.
| @@ -0,0 +1,193 @@ | |||
| import React, {FunctionComponent, useEffect, useRef} from 'react'; | |||
|
|
|||
| export const ContentConfig = { | |||
There was a problem hiding this comment.
To me, the name ContentConfig implies it's the configuration itself, but this seems to be more like a config-preset, or a predefined configuration name.
| | DefaultContentItem; | ||
|
|
||
| export type PlaceContentConfigProps = { | ||
| contentConfig: ContentConfig; |
There was a problem hiding this comment.
this should be named preset or similar.
| * Required only if type is 'custom'. | ||
| * The array lists the content elements to display. | ||
| */ | ||
| customContent?: Array<ContentItem>; |
There was a problem hiding this comment.
Instead of customContent, this could be called contentItems. And I think it would be better to simplify this a bit. So instead of
customContent={[
{attribute: 'foo'},
{attribute: 'bar'},
{attribute: 'baz', additional: 123}
]}
that would be:
customContent={[
'foo',
'bar',
{type: 'baz', additional: 123},
]}
So essentially, convert PlaceDetailsContentItem into an enum-like type (like ContentConfig above) for the child-elements without extra options and for those with options, a simple object with type + options. An alternative might be using an array like ['baz', {additional: 123}] for the configuration with options, but I'm not sure if I like that more...
| case ContentConfig.STANDARD: | ||
| return <gmp-place-standard-content />; | ||
| case ContentConfig.ALL: | ||
| return <gmp-place-all-content />; | ||
| case ContentConfig.CUSTOM: |
There was a problem hiding this comment.
I think the last case can be left away, we should use defaults for the props if possible (so without any props it would be the 'standard' preset). For the custom preset, we can just check if content items are specified.
| /** | ||
| * Content config. Defaults to 'standard' | ||
| */ | ||
| contentConfig: ContentConfig; | ||
| /** | ||
| * Required only if type is 'custom'. | ||
| * The array lists the content elements to display. | ||
| */ | ||
| customContent?: Array<ContentItem>; | ||
|
|
There was a problem hiding this comment.
see my remarks in place-content-config.tsx.
| useState | ||
| } from 'react'; | ||
| import {createPortal} from 'react-dom'; | ||
| import {usePropBinding} from '../../../../src/hooks/use-prop-binding'; |
There was a problem hiding this comment.
this could cause problems should someone try to run this example standalone. I think it's better to copy those hooks into the example directory (maybe just like utility-hooks.ts, should be just a couple of lines without further dependencies). We’ll remove them again when the components get merged into the main library.
|
One more thing, the components can in part be styled using css-variables, should we provide an interface for those as well, so users can style them using |
2 participants
Using dedicated components for the Places UI Kit components
see #878