Skip to content

feat(ew): move chat's AI disclaimer outside its bordered body#580

Open
magdalenaorlowska wants to merge 1 commit into
adobe:mainfrom
magdalenaorlowska:ewdisc
Open

feat(ew): move chat's AI disclaimer outside its bordered body#580
magdalenaorlowska wants to merge 1 commit into
adobe:mainfrom
magdalenaorlowska:ewdisc

Conversation

@magdalenaorlowska

Copy link
Copy Markdown
Contributor

Adds an optional styled property to nx-chat, reflected as the styled-as-panel attribute. When set, nx-chat wraps its header, messages, and input in a bordered/rounded/backgrounded .chat-body, and renders the AI disclaimer below it, outside that border — all within its own shadow DOM. styles.css removes the outer panel's own background, border, and border-radius whenever it contains an element with the styled-as-panel attribute, so the two don't stack. panel.js is unchanged: it has no knowledge of chat or any specific consumer, just a generic :has([styled-as-panel]) check.

Adds an optional `styled` property to nx-chat, reflected as the `styled-as-panel` attribute. When set, nx-chat wraps its header, messages, and input in a bordered/rounded/backgrounded `.chat-body`, and renders the AI disclaimer below it, outside that border — all within its own shadow DOM.
styles.css removes the outer panel's own background, border, and border-radius whenever it contains an element with the `styled-as-panel` attribute, so the two don't stack. panel.js is unchanged: it has no knowledge of chat or any specific consumer, just a generic `:has([styled-as-panel])` check.
Comment thread nx2/blocks/chat/chat.js
thinking: { type: Boolean },
connected: { type: Boolean },
toolCards: { type: Object },
styled: { type: Boolean, reflect: true, attribute: 'styled-as-panel' },

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.

styled is a boolean, we prefer to prefix those with is/has, conventionally. the prop/attr mismatch is also confusing. why not isPanel: { type: Boolean, reflect: true, attribute: 'styled-as-panel' } so the intent is clear on both sides?

Comment thread nx2/blocks/chat/chat.css
:host([styled-as-panel]) .chat-body {
border: 1px solid var(--s2-gray-100);
border-radius: var(--s2-corner-radius-800);
background: light-dark(#fff, #000);

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.

we use tokens for colors everywhere else in this file. is there a reason not to use light-dark(var(--s2-gray-50), var(--s2-gray-900)) (or whatever the white/black tokens are) here?

@sharanyavinod sharanyavinod 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.

Tbh I dont agree with this approach - styling is a pure ux concern and this shouldnt be creeping into the core logic nor requiring a reactive property imo. Should be either controlled by classes or css properties. The panel can then check for panel-wrapper:has(nx-chat) or something similar and we only touch the css. Why not go in this direction?

@hannessolo

Copy link
Copy Markdown
Contributor

To add to @sharanyavinod 's comment above - I'm also not sure we should be doing this. We built the panels system to be generic but in the end if it's only being used in 2 places so far - the chat and the right panel. If one of those places already becomes an exception, we should be wondering if that generic approach makes sense at all or if we need to build it in a different way.

So I guess my question is:

  1. Do we really need this? I agree it looks better but wonder if it's worth the effort.
  2. If we do really need this, are we ok with this approach or do we restructure the panels?

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.

4 participants