BotFramework-WebChat icon indicating copy to clipboard operation
BotFramework-WebChat copied to clipboard

Send box renderer

Open felixvanleeuwen opened this issue 5 years ago • 22 comments

Fixes #3526 Fixes #3038

Description

Added a prop that is sent down into BasicWebChat and is used to render the send box if not undefined.

Specific Changes

Added a prop to BasicWebChat defaultProps. Added a check in the BasicWebChat render to use the new prop instead of rendering the BasicSendBox

Review Checklist

This section is for contributors to review your work.

  • [ ] Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • [ ] Browser and platform compatibilities reviewed
  • [ ] CSS styles reviewed (minimal rules, no z-index)
  • [ ] Documents reviewed (docs, samples, live demo)
  • [ ] Internationalization reviewed (strings, unit formatting)
  • [ ] package.json and package-lock.json reviewed
  • [ ] Security reviewed (no data URIs, check for nonce leak)
  • [ ] Tests reviewed (coverage, legitimacy)

felixvanleeuwen avatar Oct 12 '20 09:10 felixvanleeuwen

CLA assistant check
All CLA requirements met.

ghost avatar Oct 12 '20 09:10 ghost

Thanks for the PR!!

  1. Please fill out the CLA
  2. Could you add the passing snapshots to the committed files? They need to be present in order for the CI to pass.

I'll take a closer look at this sometime this week.

corinagum avatar Oct 12 '20 21:10 corinagum

I've updated the snaps but the CI still fails. When I try to check the "Details" I get a 401.

How do I check why my CI is failing?

felixvanleeuwen avatar Oct 16 '20 11:10 felixvanleeuwen

@felixvanleeuwen thanks for the updates! Unfortunately non-internal developers are unable to check the CI. Some of them are DL timeouts, so I'm rerunning right now and after that I'll make a list of the failing tests. Thanks for your patience.

corinagum avatar Oct 16 '20 17:10 corinagum

@compulim ping

corinagum avatar Oct 16 '20 17:10 corinagum

Any news on the failing CI?

felixvanleeuwen avatar Oct 20 '20 11:10 felixvanleeuwen

Both failing tests were issues resolved yesterday. Merged main in and restarted CI.

corinagum avatar Oct 20 '20 17:10 corinagum

CI still fails, anything I can solve?

felixvanleeuwen avatar Oct 23 '20 09:10 felixvanleeuwen

CI is failing on my PR too -- both have lots of failures and look to be the same. Something else is going on, which I'll investigate today. Are you able to run the tests locally? Will you be able to change to middleware for this PR?

corinagum avatar Oct 23 '20 16:10 corinagum

Are you able to run the tests locally?

Yes I can. I have to run them on a machine that is different from my regular work station so it'll take some extra effort.

Will you be able to change to middleware for this PR?

In the current way it's implemented returning false from the sendBoxMiddleware would have the BasicWebChat default to the BasicSendBox. How would you like me to change it?

felixvanleeuwen avatar Oct 26 '20 06:10 felixvanleeuwen

Will get back today, I got some ideas on how to make it better.

One thing, can you please look at our other middleware to understand how it works? Web Chat use middleware pattern for custom-rendering components.

For example, reaction button sample demonstrates how to custom-render activity. It can be similarly applied to custom-render of send box, so developers can add a new button to the left of the upload button.

compulim avatar Oct 26 '20 12:10 compulim

I think I can look at any createCoreMiddleware to get a general idea of how it should work. Hopefully I can set some time aside next week to work on this.

felixvanleeuwen avatar Oct 27 '20 09:10 felixvanleeuwen

Starting work on this

felixvanleeuwen avatar Nov 11 '20 10:11 felixvanleeuwen

@compulim Is this how you imagined it?

felixvanleeuwen avatar Nov 11 '20 15:11 felixvanleeuwen

@felixvanleeuwen Thanks for the updates! FYI, compulim is currently on vacation and will be unavailable for a few weeks. Since he wanted to go through the code, I'll go through and possibly leave a few comments, but we'll wait until he's back for the approval.

We just finished a release milestone, and our next is scheduled for next year, so presumably we'll still be able to get this into the next release.

Thanks for your patience!

corinagum avatar Nov 11 '20 21:11 corinagum

You don't need to do this immediately, since there will be frequent changes to CHANGELOG.md that will cause merge conflicts, but before this gets merged in, you will need to give yourself credit and add the changes to the Changelog :) I would include a line under Added in Unreleased for the SendBox changes, and an entry under Samples for the new sample.

Please also add this sample to the /samples/README.md table. Thanks again :)

corinagum avatar Nov 11 '20 21:11 corinagum

Hello, is there anything blocking about this PR still? I'd like to hear what's wrong so I can take steps to remedy.

felixvanleeuwen avatar Dec 24 '20 10:12 felixvanleeuwen

Hello?

felixvanleeuwen avatar Jan 12 '21 16:01 felixvanleeuwen

Hi @felixvanleeuwen sorry I missed your reply. We still need @compulim to do a pass review, but there are still issues unaddresed above that should be fixed. Please take a look at those.

corinagum avatar Feb 09 '21 23:02 corinagum

I think all of the issues have been addressed. If you feel that some have not been addressed: could you specifically list them?

felixvanleeuwen avatar Feb 10 '21 09:02 felixvanleeuwen

Reopening - I'm assuming close was unintentional :) Please let me know if not.

corinagum avatar Mar 24 '21 19:03 corinagum

Sorry for bringing up this PR again, but is there still any interest in being able to implement a custom search box? From a developers perspective it makes perfect sense to have a middleware for this just like we have for all other crucial parts of the UI.

Or is there some sample of solving it with current tools, as I saw that there's a hideSendBox style option?

sibbl avatar Apr 21 '22 11:04 sibbl

Obsoleted by #5120.

compulim avatar Apr 08 '24 20:04 compulim