-
Notifications
You must be signed in to change notification settings - Fork 34
feat(MessageBar): pulled in compass styles #749
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
Changes from 3 commits
2e208a6
bbbce25
5107464
66dfc98
37066d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import { FunctionComponent, useState } from 'react'; | ||
| import { MessageBar } from '@patternfly/chatbot/dist/dynamic/MessageBar'; | ||
|
|
||
| export const ChatbotMessageBarIndicatorThinking: FunctionComponent = () => { | ||
| const [isThinking, setIsThinking] = useState(false); | ||
| const handleSend = (_message: string | number) => { | ||
| setIsThinking(true); | ||
|
|
||
| setTimeout(() => { | ||
| setIsThinking(false); | ||
| }, 10000); | ||
| }; | ||
|
|
||
| return <MessageBar onSendMessage={handleSend} hasAiIndicator isThinking={isThinking} />; | ||
| }; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -314,6 +314,16 @@ To enable the stop button, set `hasStopButton` to `true` and pass in a `handleSt | |||||
|
|
||||||
| ``` | ||||||
|
|
||||||
| ### Message bar with indicator and animation | ||||||
|
|
||||||
| You can pass the `hasAiIndicator` property to the `<MessageBar>` to give it a more pronounced AI indicator style. You can also pass the `isThinking` property to enable a "thinking" animation. | ||||||
|
||||||
| You can pass the `hasAiIndicator` property to the `<MessageBar>` to give it a more pronounced AI indicator style. You can also pass the `isThinking` property to enable a "thinking" animation. | |
| To add a more pronounced AI indicator style to the message bar, pass `hasAiIndicator` to the `<MessageBar>` component. You can also enable a "thinking" animation by passing in `isThinking`. |
Outdated
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.
@edonehoo mind seeing if this needs a little Hocus Pocus?
Outdated
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.
| The following example shows a simplified way of how you might hanbdle the "thinking" animation: when you send a message, the `isThinking` property is set to `true`, then after 10 seconds it is set back to false to disable the animation again. | |
| This example shows a simplified method of handling the "thinking" animation: after a user sends a message, the `isThinking` property is set to `true` to trigger the animation, then returns to `false` after 10 seconds to halt the animation. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,16 +26,18 @@ | |
|
|
||
| overflow: hidden; | ||
|
|
||
| &.pf-m-primary { | ||
| box-shadow: inset 0 0 0 1px var(--pf-t--global--border--color--default); | ||
| } | ||
| &:not(.pf-v6-m-thinking) { | ||
| &.pf-m-primary { | ||
| box-shadow: inset 0 0 0 1px var(--pf-t--global--border--color--default); | ||
| } | ||
|
|
||
| &:hover { | ||
| box-shadow: inset 0 0 0 1px var(--pf-t--global--border--color--default); | ||
| } | ||
| &:hover { | ||
| box-shadow: inset 0 0 0 1px var(--pf-t--global--border--color--default); | ||
| } | ||
|
|
||
| &:focus-within { | ||
| box-shadow: inset 0 0 0 2px var(--pf-t--global--color--brand--default); | ||
| &:focus-within { | ||
| box-shadow: inset 0 0 0 2px var(--pf-t--global--color--brand--default); | ||
| } | ||
|
Comment on lines
+29
to
+40
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mcoker this is to prevent box-shadows from blocking out the animation. Was also thinking we could apply some static class to indicate "this message bar will think" ("pf-m-think", sorta like how PF has pf-m-progress and pf-m-in-progress?), which we could have some prop for. Would we want to recommend something like that, though?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... just curious - do you know why that style was created as a box shadow and not a border? I wonder if it's because the border width needs to change dynamically and you don't want it to cause the message bar to shift? If we want to keep that border from showing up when it has
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There were issues that arose when using a border I believe, @rebeccaalpert could explain that better, but yeah the border width changing would at least be one reason. Do you mean apply a plain variation to the MessageBar wrapper? Then from the consumer end, they'd need to pass aprop to apply that plain styling/class as well as the thinking class?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry I had this and the Another thing it could do is create a single .pf-chatbot__message-bar {
&.pf-m-v6-thinking {
// box-shadow overrides
// anything else that may come up
}
}Can you think of any other benefits of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not off the top of my head no, was just curious about this specific scenario really.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, I would just stick with what you have then. FWIW on the border/box-shadow question, @srambach left a comment about that here - https://github.com/patternfly/chatbot/pull/734/files#r2482294423. So we'll probably want to make this a border instead of a box shadow at some point. And if the problem was with the border-width dynamically changing and impacting the message bar layout, you could use a pseudo element to create the border and absolutely position it relative to the message bar.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool 🫘 , sounds good! |
||
| } | ||
|
|
||
| &-actions { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| export function css(...args: any): string { | ||
| // Adapted from https://github.com/JedWatson/classnames/blob/master/index.js | ||
| // Copied from patternfly/react-styles | ||
| const classes = [] as string[]; | ||
| const hasOwn = {}.hasOwnProperty; | ||
|
|
||
| args.filter(Boolean).forEach((arg: any) => { | ||
| const argType = typeof arg; | ||
|
|
||
| if (argType === 'string' || argType === 'number') { | ||
| classes.push(arg); | ||
| } else if (Array.isArray(arg) && arg.length) { | ||
| const inner = css(...(arg as any)); | ||
| if (inner) { | ||
| classes.push(inner); | ||
| } | ||
| } else if (argType === 'object') { | ||
| for (const key in arg) { | ||
| if (hasOwn.call(arg, key) && arg[key]) { | ||
| classes.push(key); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return classes.join(' '); | ||
| } |
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.
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.
Pushed all the mentioned suggestions. Only deviation I did was using "styles' for the example title to sorta indicate there's more than 1 thing going on in the example
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.
makes sense, that's fine with me!