-
Notifications
You must be signed in to change notification settings - Fork 37
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(components): implement ui components package #208
Conversation
packages/react-message-renderer/src/renderer/QuickReplies/index.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.
What do you think about renaming the folder to components
and renaming the package to @botpress/messaging-components
?
Do this close botpress/botpress#5476? |
…210) * Rename folder to components * rename package to @botpress/messaging-components
I updated the description, this is ready for a first version |
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.
Small comments otherwise it looks good to me. Awesome work here 👌
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.
Not Done. But here's the first batch of comments
roots: ['.'], | ||
transform: { | ||
'^.+\\.tsx?$': 'ts-jest', | ||
'.+\\.(css|styl|less|sass|scss)$': 'jest-css-modules-transform' |
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.
Not that it changes anything but you're shooting quite large here 🤣
} | ||
} | ||
|
||
export function pick<T>(obj: T, keys: Partial<keyof T>[]): Partial<T> { |
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 imagine you didn't want to import lodash for a reason ?
It's treeShakable if you import by function
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.
It's a simple function and I wanted to keep the bundle size as low as possible
return html.replace(/<a href/gi, '<a target="_blank" href') | ||
} | ||
|
||
export class FallthroughIntl implements InjectedIntl { |
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.
Why this ?
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 didn't want to manage and load translations here but one or two of the renderers calls intl for translation, I just made a dummy class that implements the interface, you can override it in the messageconfig object with your own instance if needed!
noMessageBubble: false, | ||
intl: new FallthroughIntl(), | ||
showTimestamp: false, | ||
bp: window.botpress, |
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 imagine it would be possible for a user to use this library without being on the botpress studio meaning that window.botpress will be undefined.
What happens if it's not set ? do you make heavy use of it ?
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.
this is just for custom components, we do a check and throw an error if it is not defined:
const InjectedModuleView = config.bp?.getModuleInjector() |
|
||
type CardProps = CardPayload & Pick<MessageConfig, 'onSendData'> | ||
|
||
export const Card = ({ picture, title, subtitle, buttons, onSendData = async () => {} }: CardProps) => { |
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.
Is this the same card as the card component ? if not .. why not ?
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.
When you create a card in botpress you are creating a single card carousel! There is no card renderer in botpress only carousel
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.
Oh, Gotcha !
return ( | ||
<a | ||
href={btn.url} | ||
key={`1-${btn.title}`} |
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.
curious about why prefix the keys if you handle all cases one by one as if else ? Any chance of duplicates ?
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.
This is from copy pasting, left it as is, I could hash the payload and use that as a key
import Keyboard from '../Keyboard' | ||
import ErrorBoundary from './ErrorBoundary' | ||
|
||
const checkError = (moduleInjector: Function, payload: CustomComponentPayload): Error | null => { |
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.
Should we unit test this ?
return null | ||
} | ||
|
||
export const CustomComponentRenderer: React.FC<MessageTypeHandlerProps<'custom'>> = ({ config, payload }) => { |
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.
would be nice to have another way of providing custom components ....
Oh wait, can a user override the custom component renderer ?
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.
That's a way of doing it! I plan on improving the Renderer
class in the future so that adding a new message type is simpler, it is hard to do well while keeping the types fully checked
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.
Right now you need to add your new type to the messageTypes array and add a default renderer that satisfies MessageTypeHandler<'my_new_message_type'>
@charlescatta Is the integration with HITL HITL Next and misunderstood being done simultaneously? Or do we need to merge this PR before doing that? |
we need to merge first |
@samuelmasse What is the reason behind the dependencies changes? |
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.
Alright this PR has been in review for long enough. @charlescatta good job with the package, this should make sharing logic between different uis a lot cleaner. We can merge this right now and start using it internally.
In my opinion it's a waste of time to review something that isn't being used for it's designed purpose. Once this package starts being used for hitl, webchat, misunderstood, etc. we'll have a better view of what needs to be improved.
I fixed some problems with peer depencies. It turns out babel was used somewhere apparently, because yarn was displaying huge amounts of peer dependency warning. I also changed the react version back to 16.8 because most packages we displaying peer-depency errors aswell for version 17.
We'll need other PRs to change the setup of the project a bit as I noted in my comments. Personnally I don't think this is too important for now and either me or @laurentlp can do these changes. What I'd like to see soon is practical usage of this package!
@@ -0,0 +1,1089 @@ | |||
/* Old version IE 10 and 11 only */ |
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.
Where was this copy pasted from. Is the file at the original source still there? We should avoid copy pasted code if possible.
# TODO: Merge all tests into root jest config and remove this | ||
- name: Run Components tests | ||
run: | | ||
yarn workspace @botpress/messaging-components test |
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.
This can be done in a future PR
transform: { | ||
'^.+\\.tsx?$': 'ts-jest', | ||
'.+\\.(css|styl|less|sass|scss)$': 'jest-css-modules-transform' |
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.
In a future PR this should be moved to the root config. Our test setup isn't very well adjusted for many packages at the moment so we'll see how we do this.
import '@testing-library/jest-dom/extend-expect' | ||
import './src/mocks/matchMedia.mock' // required by react-slick | ||
|
||
export default {} |
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.
Why a setup and config file?
"@babel/core": "^7.15.8", | ||
"@storybook/addon-essentials": "^6.3.12", | ||
"@storybook/addon-links": "^6.3.12", | ||
"@storybook/addon-measure": "^6.4.0-alpha.27", | ||
"@storybook/components": "^6.3.12", | ||
"@storybook/core-events": "^6.3.12", | ||
"@storybook/react": "^6.3.12", |
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'm not sure where babel is used but it was disaplying an annoying "unmet peer dependency" message as well as with other packages. There's still some of these messages left. For example fixing the unmet peer dependency "babel-loader" creates another peer dependency that requires us to install webpack...
const input = 'src/index.tsx' | ||
|
||
const external = [...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {})] | ||
|
||
const plugins = [ | ||
typescriptPlugin({ | ||
typescript | ||
}), | ||
css() | ||
] |
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.
We'll have to see how the development flow goes with this package. Ideally we should be able to have at hot-reloadable ui that uses this package (the webchat for example). I know how to do this with parcel, we'll see if we can do the same with rollup
.slick-prev, | ||
.slick-next { | ||
font-size: 0; |
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.
What is this file for? What's slick?
"outDir": "build", | ||
"module": "esnext", | ||
"lib": ["dom", "es2017"], | ||
"sourceMap": true, | ||
"jsx": "react", | ||
"moduleResolution": "node", | ||
"resolveJsonModule": true, | ||
"target": "es5", | ||
"skipLibCheck": false, | ||
"allowSyntheticDefaultImports": true, | ||
"strict": true, | ||
"forceConsistentCasingInFileNames": true, |
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.
This file doesn't follow the format we have for our others tsconfig files. To be fixed in future PR
This will allow modules like
HITL
HITL Next
andmisunderstood
to render messages only by having their payload.The default export of the package is the
renderMessage
function. Here is an example usageTODOS
Renderer Tests
Other stuff
Double check typings by logging payload from channel web renderers:
Future improvements
messaging/server
andchannel-web