-
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
Conversation
|
Preview: https://chatbot-pr-chatbot-749.surge.sh A11y report: https://chatbot-pr-chatbot-749-a11y.surge.sh |
| &: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); | ||
| } |
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.
@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?
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.
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 -m-thinking (or it's wrapped in something with -m-thinking), this looks like a good candidate for a plain variation since we're effectively creating a separate border for it. Would that work?
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 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?
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.
Oh sorry I had this and the ai-indicator class confused and thought this was a border style. I think your style block is fine. Having a "will think" class could be good, but the only benefit I can think of is that if we end up needing a .pf-m-think down the road and users will need to add it to get the correct thinking classes, that's already a requirement now (not breaking) instead of a new requirement later on (probably breaking?).
Another thing it could do is create a single .pf-m-think class where you can bundle all of the styles that need to change for .pf-v6-m-thinking instead of sprinkling :not(.pf-v6-m-thinking) throughout existing styles, but you could do that currently by just having a block that uses .pf-v6-m-thinking like this
.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 .pf-m-think that I'm not considering?
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.
Can you think of any other benefits of .pf-m-think that I'm not considering?
Not off the top of my head no, was just curious about this specific scenario really.
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.
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.
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.
Cool 🫘 , sounds good!
| 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. | ||
|
|
||
| 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. |
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?
edonehoo
left a comment
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.
some shuffling and small suggestions - lmk what you think!
|
|
||
| ### 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. |
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.
| 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`. |
|
|
||
| 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. | ||
|
|
||
| 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. |
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. |
|
|
||
| ``` | ||
|
|
||
| ### Message bar with indicator and animation |
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.
| ### Message bar with indicator and animation | |
| ### Message bar with AI indicator style |
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!
kmcfaul
left a comment
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.
react implementation lgtm
mcoker
left a comment
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.
L🌈TM!
|
🎉 This PR is included in version 6.5.0-prerelease.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #727
New example: Message bar with indicator and animation