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

Fixed chat send history functionality in Firefox and Edge #1956

Merged
merged 3 commits into from
May 2, 2019

Conversation

tdurnford
Copy link
Contributor

@tdurnford tdurnford commented May 2, 2019

Fixes #1948

Currently, in the Send History Sample, there is a conditional checking if the send box text input's selection start is positioned at index 0. If it is, the user can navigate through their previous messages; otherwise, the chat history functionality is not accessible.

When the send box value is set from history in Chrome the cursor is placed at the begin of the string; however, it is placed at the end of the string in Firefox and Edge. Consequently, target.selectionStart === 0 will be true in Chrome after the text input value is set, but not in the other two browsers. Thus, if the user wanted to toggle through the chat history in Firefox or Edge, they would have to place the cursor at the beginning of the string after they press the up arrow since target.selectionStart === 0 would be false.

The conditional is intended to prevent users from toggling through the chat history if they are already typing a message; however, the isDirty state property already tracks if the user has typed something into the send box. Removing the condition allows users to toggle through the messages in Firefox and Edge and does not affect the rest of the sample's functionality.

@tdurnford tdurnford marked this pull request as ready for review May 2, 2019 17:39
@coveralls
Copy link

coveralls commented May 2, 2019

Coverage Status

Coverage increased (+0.4%) to 58.154% when pulling fa81cbc on tdurnford:1948 into 6f67e58 on Microsoft:master.

@compulim
Copy link
Contributor

compulim commented May 2, 2019

The change looks good. But the sample itself isn't quite, not your fault.

We shouldn't capture onKeyDown on the whole Web Chat. Very hacky sample.

Our sample is intended for our users to copy and it will work. I won't say it will be in production quality, but they can't be hacky too. Our sample is "our best practice to do one small thing at a time, in a very correct way". And it also serve as a teaching material for our users. Then we evaluate the feedback and may fold the code back into production.

If our sample is hacky, few things would happen:

  • Devs spot we are hacky, they will not trust our product anymore
    • In recent years, I saw devs upset about quality of our samples/documentations and complaining on social media, and more people start leaving our platform
  • Devs don't know it is hacky, when they aggregate multiple samples together, everything will start falling apart
  • We cannot fold the sample code back into our product
  • Devs learn wrong thing, and the ripple effect, they will pretty soon start creating a monster that is difficult to maintain for them

On one side, to me it's a smaller side, I am worried about product quality will drive users away.

On the other side, which is bigger to me, I am worried about devs training and quality. If they can do better at their job, they will be able to create a more sounding solution that would evolve and last.

And to me, being passionate about customers, teach them, advocate great things, empower them, is something in Microsoft's DNA. We ran MSDN documentation and it was every dev's go-to place to learn things. The community changed, MSDN changed, but the spirit never.

Too much whine.

The better solution for this sample, one (baby) step at a time:

  • Build a React component, independent of Web Chat, that would work with history using up/down arrow, etc.
    • It doesn't need to be live inside its own repository or package
  • Build a simple React app that works with this component, still, independent of Web Chat
  • Improve the React component by trying out both <input> and <textarea> (multiline)
  • Build and customize Web Chat, modify send box and replace <input> with this component

For production quality, add automated tests, wrap the component nicely, etc.

@corinagum corinagum merged commit 73cef19 into microsoft:master May 2, 2019
@tdurnford tdurnford deleted the 1948 branch June 3, 2019 21:59
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.

17.chat-send-history only retrieves one previous send in Firefox and Edge
4 participants