-
Couldn't load subscription status.
- Fork 115
feat: Configurable chat icon #1853
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
|
Let's make sure to make the relevant server-side changes to shinychat (before or while merging this) |
|
@gadenbuie since it kinda felt like we'd eventually want to ability to change icons during a chat session, I took a quick stab in fbce8d2. Lmk what you think. |
Currently used to manage the assistant icon, but this pattern can be used for other icons as well.
* `<shiny-chat-container icon-assistant="...html for icon...">` * `<shiny-chat-message icon="...html for icon...">`
f0ac89d to
0d64647
Compare
efdd8cb to
f252c2c
Compare
| } | ||
|
|
||
| class ChatContainer extends LightElement { | ||
| @property({ attribute: "icon-assistant" }) iconAssistant = ""; |
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.
I don't feel strongly, but have been using snake_case for other attributes
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.
Yeah, I was a little confused about this actually because it's icon_assistant in the Tag() call. Are you explicitly forcing snake case for content_type somewhere?
I'd say my preference for kebab case attributes trends toward strong (I didn't care enough to figure out why it's content_type and not content-type, but if I definitely wanted to change it when I saw it).
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.
I'm not sure I understand the question and the proposal here. Btw, since it's probably important context, shiny this behavior when writing tags:
>>> from shiny import ui
>>> ui.div(foo_bar = "baz")
<div foo-bar="baz"></div>
>>> ui.div(fooBar = "baz")
<div fooBar="baz"></div>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.
Yeah, this is why I was surprised you have underscores in the attributes. So I used Tag(..., icon_assistant=str(icon_assistant)) in Python, which creates icon-assistant="...html...".
But you're saying you used snake_case for attributes like content_type, which means you had to do something special on the Python side, right?
As far as proposals, I'd prefer we pick one of these choices in order from first to last preference:
kebab-casefor attributescamelCasefor attributes if we want to signal that they're reflected attributessnake_casefor attributes if we want to signal that they came from Python
(I could be convinced into 2 or 3 if we have some guidelines for consistency, otherwise I'd much rather all attributes in kebab-case.)
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.
I'm still a bit confused, can we be a bit more specific? AFAIK, I'm using Tag(content_type="") in Python which serializes to <tag content-type=""> in HTML. In the TS, the property name is content_type, but the attribute name is content-type. Are you proposing kebab-case everywhere (even for the TS property names)?
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.
For the chat messages you pass JSON data (which has content_type) directly from the event to create the message element, so the <shiny-chat-message> (and similar) elements have content_type attributes, although when creating the <shiny-markdown-stream> you then go from content_type to content-type.
Anyway, I've always personally preferred:
- Snake case in R/Python, i.e.
Tag("shiny-chat-message", content_type="html") - Kebab-case in HTML, i.e.
<shiny-chat-message content-type="html"> - camelCase in TS/JS, i.e.
this.contentType.
But the event-data-direct-to-custom-element approach is easy and it would take a little more work to serialize those elements to JSON with kebab-case names.
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.
Ahh, I overlooked the message handling aspect 🤦 .
Would it make sense to you if createElement() inside of js/utils/_utils.ts replaced _ with - (similar to what htmltools in Python does)?
Anyways, I don't think I have any strong opinions here and align with your preferences, so feel free to change as you see fit
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.
Looks good once suggestions are addressed, thanks!
Co-authored-by: Carson Sievert <[email protected]>
Adds
icon_assistanttochat.ui()andui.chat_ui()that can take a tag, typically an<svg>(e.g.faicons.icon_svg()or wrapped inui.HTML()) or an<img>.For example, using
faicons:Or directly using an image: