Skip to content

feat(MessageDivider): add MessageDivider, example, tests - #598

Merged
rebeccaalpert merged 10 commits into
patternfly:mainfrom
kmcfaul:message-divider
Jul 28, 2025
Merged

feat(MessageDivider): add MessageDivider, example, tests#598
rebeccaalpert merged 10 commits into
patternfly:mainfrom
kmcfaul:message-divider

Conversation

@kmcfaul

@kmcfaul kmcfaul commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

Closes #44

First pass on MessageDivider.

  • Adds MessageDivider component with two variants: "date" and "announcement"
  • Defaults to date variant and will put the current date as the content similar to the Message default timestamp

The example is on the Messages page: https://chatbot-pr-chatbot-598.surge.sh/patternfly-ai/chatbot/messages.

Open questions:

  • CSS/overall structure (need to do a second pass on using tokens, I think there are still some hardcoded values that can be replaced)
  • Is there a way to apply a pf css class? pf-c-v6-divider would also work when applied to the date variant class element and some css could be removed if that's the case, but I was unable to get the class to apply in a vacuum.
  • Should the "date" variant be named something more generic since any text could be passed?
  • Should children be used instead of content? I was following Message using content but unsure if we would want to support other types beyond string.
  • What should the padding for the divider be?

@kmcfaul
kmcfaul marked this pull request as draft July 9, 2025 14:38
@patternfly-build

patternfly-build commented Jul 9, 2025

Copy link
Copy Markdown

@kmcfaul

kmcfaul commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

@kaylachumley What's your opinion for the padding on the divider? I didn't want to have it bump up against the Messages so I gave it 12px of padding on the top/bottom to start with, but Message already has a lot of bottom padding so I wasn't sure if that's too much.

@kmcfaul

kmcfaul commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

@mcoker Would love your eyes on this, since I'm not 100% on whether there's a better way to do the CSS for the date variant especially. I essentially took the pf-c-v6-divider styles to get the divider to render, then made that position: relative while it's child that contains the text content has position: absolute with an offset to push it back on top of the divider line. It's currently very tied to being 24px tall because of this.

@rebeccaalpert rebeccaalpert linked an issue Jul 9, 2025 that may be closed by this pull request
@rebeccaalpert
rebeccaalpert requested a review from edonehoo July 9, 2025 20:15
@kmcfaul
kmcfaul marked this pull request as ready for review July 10, 2025 17:05

@rebeccaalpert rebeccaalpert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am super excited about this. I left some comments below. The big one is something I should have noticed earlier - the same color is being used for the divider and ChatBot background. I'm sorry about that. Kayla gets the final say, but we can use the tertiary color for now probably.

I'm more interested in Michael Coker's approach, but this is another way to do the line that doesn't involve absolute positioning and px values (I am not using tokens here): https://codesandbox.io/p/sandbox/dwn7yx

It does have the same wrapping issue I note below if you take the no-wrap off. I think if we set a max-width on the bubble in the example above and center-align the text it will wrap nicely.

Comment thread packages/module/src/MessageDivider/MessageDivider.tsx
Comment thread packages/module/src/MessageDivider/MessageDivider.tsx Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
@kaylachumley

kaylachumley commented Jul 14, 2025

Copy link
Copy Markdown

@kaylachumley What's your opinion for the padding on the divider? I didn't want to have it bump up against the Messages so I gave it 12px of padding on the top/bottom to start with, but Message already has a lot of bottom padding so I wasn't sure if that's too much.

@kmcfaul Hey! Looks like in the demos there is 48px of padding between incoming bot and user messages, related to that, i would love to change that to be 32px of padding so its not so much space. BUT as for the padding on the divider component, in figma I dont have any extra applied to the top and bottom and just rely on the 32px padding. Here is a visual of that:

Screenshot 2025-07-14 at 12 38 21 PM Screenshot 2025-07-14 at 12 38 27 PM

@kaylachumley kaylachumley left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

thanks! left a bunch of comments

@kmcfaul

kmcfaul commented Jul 14, 2025

Copy link
Copy Markdown
Contributor Author

@kaylachumley Once the build finishes, I've updated the padding of both the message and message divider to be 32px. LMK if that looks better. I can also revert the Message padding back to 48 if we don't want to update that atm.

@kaylachumley

Copy link
Copy Markdown

@kmcfaul oh awesome! yeah as long as @rebeccaalpert is okay with updating the padding to be 32px now then im good. Can we use this token for that? --pf-t--global--spacer--xl?

@kmcfaul

kmcfaul commented Jul 14, 2025

Copy link
Copy Markdown
Contributor Author

Yep --pf-t--global--spacer--xl is the token I used for both.

@rebeccaalpert

Copy link
Copy Markdown
Member

The vertical message spacing is fine with me @kaylachumley @kmcfaul! I think we just need to communicate in the commit that the spacing changed so we have it for the release notes.

Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
@mcoker

mcoker commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

Here is this component using the label and divider components from PF - https://codepen.io/mcoker/pen/ZYbGbJY?editors=1100

Here are the modifications needed to our components to get it to match the figma. IMO everything except the updated left/right padding seems like fair stuff we could add upstream to remove these modifications if we wanted to. Maybe design could think of a way/reason to add the padding changes upstream, too?

  • Modifies the label background to use background--color--secondary since the default grey color is color--nonstatus--grey. This could exist as a variation for the PF component if we wanted that to exist upstream.
  • Modifies the label border to use border--color--default. Same - this could be an upstream thing if we wanted to make a variation for it.
  • Modifies padding to use spacer--action--horizontal--compact. It's a medium spacer, used for small buttons/toggles - seems appropriate here unless design wants to use a medium spacer specifically (or other token).
  • I added a .pf-m-wrap variation to the message-divider component that disables the <Label>'s default text truncation/tooltip behavior. If we never want the label to truncate with a tooltip, we could remove that .pf-m-wrap modifier and just put the style it sets as a default style for this component. That could also exist as a variation of the <Label> component if we wanted that upstream.
  • I made the label full-width by default using grid. This could also be upstream as a .pf-m-block modifier on <Label>.

WDYT? Want to go with something like this, or prefer to keep what's in the PR?

One additional comment - this component has a padding-bottom. If everything that goes in the message list (.pf-chatbot__messagebox it looks like?) has the same bottom padding, .pf-chatbot__messagebox could be a flex column with a gap, and you could remove the individual paddings.

And also I noticed false being rendered as a class on the .pf-chatbot__messagebox element

Screenshot 2025-07-15 at 11 23 10 AM

@rebeccaalpert rebeccaalpert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm approving what's in here as changes right now that we've previously discussed. I'd like to address the MessageBox stuff Michael Coker is raising separately since it's a separate component. I'm opening an issue here: #606.

@mcoker mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great! Nothing needs to change, though if I prioritized any of my comments, I'd love to change the 1px border on .pf-chatbot__message-divider-content to use the border-width--default token. And then see if we can remove the min-height with a pixel value and get that via top/bottom padding.

Everything else is just a nit for styles that shouldn't be necessary, though who knows - maybe necessary in some cases with what kind of app/pages people pull chat bot in? Like the font styles on message-divider-text - those should be unnecessary in a patternfly app, but maybe you need to define color and font-style because some other app's default color and font-style may be something different.

Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
Comment thread packages/module/src/MessageDivider/MessageDivider.scss Outdated
@kmcfaul

kmcfaul commented Jul 15, 2025

Copy link
Copy Markdown
Contributor Author

Before (custom css):
image

After (using Label + Divider):
image

@kmcfaul

kmcfaul commented Jul 15, 2025

Copy link
Copy Markdown
Contributor Author

Updated to use the Label + Divider based on your codepen @mcoker. Only significant changes from that should be -updating the background colors to tertiary for the full width variant, adding the text-align:center for the wrapping, and adding font-weight for the label text.

@kaylachumley @rebeccaalpert Do you mind taking another look? If there are any issues I can back out the commit. Design wise it should be the same except for a tiny difference in vertical padding (see above images, it's like 26px versus 24px for the line height). Edit: It's probably also possible to adjust the line-height as well, but atm it's using the default padding and spacing that comes from Label for the most part).

Comment thread packages/module/src/MessageDivider/MessageDivider.tsx Outdated
}: MessageDividerProps) => {
if (variant === 'inset') {
return (
<div className="pf-chatbot__message-divider pf-m-divider pf-m-wrap" {...props}>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

WDYT about the need for .pf-m-wrap? If the text should always wrap and never truncate, you could take the class off and move the .pf-m-wrap style block up into the top to L13 or so of MessageDivider.scss

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think leaving it as a separate modifier may be fine for now, in case truncation is desired later (and so the modifier controlled by a flag)

@rebeccaalpert rebeccaalpert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am just going to approve again; it looks good to me. Tested wrapping and in context of main ChatBot demo and it looked fine!

@rebeccaalpert
rebeccaalpert merged commit 38eda58 into patternfly:main Jul 28, 2025
7 checks passed
rebeccaalpert pushed a commit to rebeccaalpert/virtual-assistant that referenced this pull request Oct 24, 2025
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.

Add Separator components

6 participants