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

chore: Fix bugs - external CSS leaks, sending empty queries, CSSStylesheet constructor error on mobile #21

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

mrderyk
Copy link
Contributor

@mrderyk mrderyk commented Mar 1, 2024

CONTEXT

We have a number of bugs that need to be addressed:

  • external CSS leaking into the shadow DOM of the react-chatbot web component
  • users able to send a query even if the text input field is empty
  • the chatbot component breaking on mobile browsers since mounting it inside a web component

CHANGES

  • Introduce :host selector styling to reset all styles in the web component's shadow host
  • Short-circuit text input submit handler if the trimmed query is of length 0
  • Rename QueryInput isDisabled prop to isButtonDisabled since only the button uses it i
  • Set QueryInput isButtonDisabled prop to true if waiting for a chat response or trimmed query is of length 0
  • When constructing web component styles, fall back to style element if CSSStylesheet constructor is not supported

@mrderyk mrderyk changed the title Fix/misc bugs chore: Fix bugs - external CSS leaks, sending empty queries, CSSStylesheet constructor error Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-03-01 22:39 UTC

@mrderyk mrderyk changed the title chore: Fix bugs - external CSS leaks, sending empty queries, CSSStylesheet constructor error chore: Fix bugs - external CSS leaks, sending empty queries, CSSStylesheet constructor error on mobile Mar 1, 2024
@mrderyk mrderyk requested a review from cjcenizal March 1, 2024 19:06
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Code looks good! Had one minor non-blocking suggestion. Didn't test locally.

@@ -112,7 +112,7 @@ export const ChatView = ({
const hasContent = isLoading || messageHistory.length > 0;

const onSendQuery = useCallback(() => {
if (isLoading) return;
if (isLoading || query.trim().length === 0) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can extract out a variable to indicate the coupling between the disabled appearance and the disabled behavior:

const isRequestDisabled = isLoading || query.trim().length === 0;

Is the useCallback saving any render cycles here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good callout! onSendQuery used to be a bit more complex, justifying the useCallback- but it doesn't need it anymore.

Addressed these comments here: a66a7f6

@mrderyk mrderyk merged commit f59c116 into main Mar 1, 2024
4 checks passed
@mrderyk mrderyk deleted the fix/misc_bugs branch March 1, 2024 22:39
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.

2 participants