Skip to content
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(react): coverComponentProps #899

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Conversation

AinaVendrell
Copy link
Contributor

Remember to tag the PR with WIP tag if it's not ready to be merged.
Reference

Description

Add coverComponent props

Context

Since the coverComponent is shown automatically the first time that a user interacts with the bot there was no possibility to pass properties to this component

Approach taken / Explain the design

Converts the coverComponent to an object that contains a component and props

To documentate / Usage example

Instead of defining the cover component as:

import CustomCover from '../webchat/custom-cover'

export const webchat = {
  coverComponent: CustomCover
} 

Now it can be defined as:

import CustomCover from '../webchat/custom-cover'

export const webchat = {
  coverComponent: {
    component: CustomCover,
    props: { ... },
  },
} 

Testing

The pull request...

  • has unit tests
  • has integration tests
  • doesn't need tests because... [provide a description]

Copy link
Contributor

@asastre asastre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AinaVendrell when you say in the description 'Now it can be defined as:' I thought that this new declaration of the coverComponent was optional and you could declare it both ways, the old and the new one, but looking into the code I can see only de new declaration working.

Since I've seen it's possible to load de correct configuration of Contentful's plugin inside the coverComponent using an environment variable and without needing to send the configuration parameters through props, I don't know if this new feature it's a must to be implemented.
In my opinion, I think it's great to have de possibility to send information to the coverComponent through props, but since it's a thing that maybe won't be much used, maybe it should be nice to have backwards compatibility and ensure that the old declaration of the coverComponent will work too.

What do you think @ericmarcos, @dpinol, @vanbasten17 ?

packages/botonic-react/src/webchat/webchat.jsx Outdated Show resolved Hide resolved
@dpinol
Copy link
Contributor

dpinol commented Sep 4, 2020

@AinaVendrell can you follow this up?

@AinaVendrell AinaVendrell force-pushed the feat/coverComponentProps branch 2 times, most recently from 2580c58 to 4fcc2f7 Compare September 28, 2020 21:54
packages/botonic-react/src/webchat/webchat.jsx Outdated Show resolved Hide resolved
packages/botonic-react/src/webchat/webchat.jsx Outdated Show resolved Hide resolved
@ericmarcos
Copy link
Contributor

@AinaVendrell I don't understand the utility of this. What can you achieve with this feature that you couldn't do before?

Copy link
Contributor

@vanbasten17 vanbasten17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asastre I also think it's nice to be able to pass props to the component. Maybe it's not very hard to implement a way to make this backward-compatible by defining the old way and having also this new definition. If it's not affordable, then I'll preserve the old compat.

@AinaVendrell AinaVendrell force-pushed the feat/coverComponentProps branch from 4fcc2f7 to 18419c5 Compare September 29, 2020 08:26
Copy link
Contributor

@asastre asastre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👏!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants