feat: add custom styling and branding v2 #160

anfibiacreativa · 2023-12-05T18:33:25Z

Purpose

...

Does this introduce a breaking change?

[ ] Yes
[x ] No

Pull Request Type

What kind of change does this Pull Request introduce?

[ ] Bugfix
[ x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

Get the code

git clone [repo-address]
cd [repo-name]
git checkout [branch-name]
npm install

What to Check

This is the custom styling and branding feature.

shibbas · 2023-12-06T04:42:33Z

packages/chat-component/src/components/chat-avatar.ts

+}
+
+@customElement('chat-avatar')
+export class ChatAvatarComponent extends LitElement {


since there is nothing in this component that's chat specific, it's a simple linked icon, should we just call it something like that? 'link-icon'. It will make it reusable for any place an clickable icon can be used.

Good suggestion. Renamed to 'link-icon'

shibbas · 2023-12-06T04:43:45Z

packages/chat-component/src/components/chat-avatar.ts

+
+  override render() {
+    return html`
+      <a class="chat-avatar__link" title="${this.label}" href="${this.url}" target="_blank" rel="noopener noreferrer">


styles inside a component are isolated and will not be impacted by outside styling of component. So you dont need a class and can style it using as a 'a' link.

That's true. I did deal so much with specificity over the years I still struggle to target native elements for styling, even inside of shadow DOM. I still do believe it may pose extensibility challenges at that granular, encapsulated level. But for the sake of avoiding premature optimization, I updated.

shibbas · 2023-12-06T04:44:32Z

packages/chat-component/src/components/chat-component.ts

@@ -61,12 +66,18 @@ export class ChatComponent extends LitElement {
  @property({ type: String, attribute: 'data-api-url' })
  apiUrl = chatHttpOptions.url;

+  @property({ type: String, attribute: 'data-custom-branding', converter: (value) => value?.toLowerCase() === 'true' })
+  isCustomBranding: boolean = globalConfig.IS_CUSTOM_BRANDING;


would be good to be consistent with 'useStream' as 'useCustomBranding'

use is a specific convention of react hooks. In general, booleans are named isX or hasX. I am not convinced about porting framework specific conventions to the rest of the development scope.

I meant more about the already in use "useStream" property just below in this component and to keep to the same standard here and not about porting react convention. But I think hasCustomBranding actually is much better.

Agreed, I'd say hasCustomBranding would make more sense

shibbas · 2023-12-06T04:47:07Z

packages/chat-component/src/components/chat-thread-component.ts

@@ -41,6 +41,12 @@ export class ChatThreadComponent extends LitElement {
  @property({ type: Object })
  selectedCitation: Citation | undefined = undefined;

+  @property({ type: Boolean })


since you are not using the branding in the thread this can be removed. Same with the icon below.

shibbas · 2023-12-06T04:50:33Z

packages/chat-component/src/styles/chat-thread-component.ts

    color: var(--error-color);
-    padding: 20px;
+    padding: var(--d-base);
    background: var(--error-color-background);


I think this was missed since hte name for error-color-background changed.

Good catch. I tried to make sure they were all updated but missed this one.

shibbas · 2023-12-06T04:51:33Z

packages/chat-component/src/styles/chat-thread-component.ts

    }
  }
+  .chat-history__footer {


this should be in the chat-component and not chat-thread-component

Removed. This was likely a merge glitch.

shibbas · 2023-12-06T04:52:09Z

packages/chat-component/src/styles/chat-component.ts

    border: 0;
    background: none;
    cursor: pointer;
    text-decoration: underline;
  }
+  @keyframes chatmessageanimation {


this was moved out of here into their respective components

shibbas · 2023-12-06T04:52:29Z

packages/chat-component/src/styles/chat-component.ts

    padding: 0;
    margin: 0;
  }
+  .subheadline--small {


same for this I think

shibbas · 2023-12-06T04:53:06Z

packages/chat-component/src/styles/chat-component.ts

-    padding: 5px 0;
-    font-size: small;
-  }
-  .chat__header {


merge issue?

This is a new wrapper to better handle reusable header.

shibbas · 2023-12-06T04:53:43Z

packages/chat-component/src/styles/chat-component.ts

+    font-size: var(--font-large);
+    border: var(--border-thin) solid var(--c-light-gray);
+    border-radius: var(--radius-large);
+    padding: var(--d-base);
  }
  .chat-history__footer {


looks like it's still here, but something is causing an issue for it to render...

I could not see the issue, maybe it has updated?

shibbas · 2023-12-06T04:54:35Z

packages/chat-component/src/components/chat-stage.ts

+  isEnabled: boolean;
+}
+
+@customElement('chat-stage')


i'm not sure this needs another component. The reuse for this is pretty small...

But it may get extended by customers. I think it doesn't hurt to keep it encapsulated.

shibbas · 2023-12-06T05:02:45Z

packages/webapp/src/pages/chat/Chat.tsx

+  });
+
+  const [customStyles, setCustomStyles] = useState(() => {
+    const styleDefaultsLight = {


duplicated? the themes folder with default will make them shared instead.

I will review this in the context of refactoring the theming, as part of this PR.

shibbas · 2023-12-06T05:05:10Z

packages/webapp/src/pages/chat/Chat.tsx

+    return () => {
+      window.removeEventListener('storage', handleStorageChange);
+    };
+  }, [customStyles, isBrandingEnabled, isDarkTheme, isLoading]);


do you need to replicate this in OneShot.tsx? If so, I think it might be better to move all this into it's own context. Ideally themes in React should be passed in as a context provider pattern so that root level themes can be passed in and can be updated like that across all the child components.

Ok, I will also handle this as part of the theming refactoring, before making this PR ready for review.

sinedied · 2023-12-11T16:14:09Z

packages/chat-component/src/components/chat-component.ts

@@ -61,12 +66,18 @@ export class ChatComponent extends LitElement {
  @property({ type: String, attribute: 'data-api-url' })
  apiUrl = chatHttpOptions.url;

+  @property({ type: String, attribute: 'data-custom-branding', converter: (value) => value?.toLowerCase() === 'true' })
+  isCustomBranding: boolean = globalConfig.IS_CUSTOM_BRANDING;


Agreed, I'd say hasCustomBranding would make more sense

sinedied · 2023-12-11T16:26:30Z

packages/chat-component/src/components/chat-component.ts

@@ -108,6 +119,26 @@ export class ChatComponent extends LitElement {

  static override styles = [chatStyle];

+  override updated(changedProperties: Map<string | number | symbol, unknown>) {


More food for thoughts: maybe we could skip that part entirely and allow to directly override the CSS variables through CSS?

Let's think about it. We can have a follow up to this.

I opened a follow up issue, btw.

#160 (comment)

I kind of disagree here, though, because it's a 'isCustomBrandingEnabled' as 'isDarkThemeEnabled'. We can assume that the app 'hasCustomBranding', wether it's on or off, regardless. But this indicates it is enabled when true.

sinedied · 2023-12-11T16:30:14Z

packages/webapp/src/components/ThemeSwitch/ThemeSwitch.tsx

+  }, [isDarkTheme, isConfigPanelOpen]);
+
+  return (
+    <>


Is the empty container needed? Since there's already a div container

perhaps something didn't get pushed? I still see the empty <>

Sync didn't happen.

manekinekko · 2023-12-11T16:41:38Z

LGTM

anfibiacreativa · 2023-12-11T18:49:52Z

@shibbas @sinedied If there are no additional change requests/blockers I would like to merge asap. Any further enhancements we can deal with in follow-up issues, as this PR is already quite massive.

shibbas · 2023-12-12T01:07:59Z

packages/chat-component/src/styles/chat-thread-component.ts

+  .chat__header--avatar {
+    display: flex;
+    align-items: center;
+  }


I think this no longer is in the thread component no?

shibbas · 2023-12-12T01:12:42Z

@shibbas @sinedied If there are no additional change requests/blockers I would like to merge asap. Any further enhancements we can deal with in follow-up issues, as this PR is already quite massive.

Yea, no blockers. There are a few things here and there that we can follow up in separate issues.

anfibiacreativa added 16 commits December 4, 2023 13:04

feat: add chat stage and avatar components

7e35096

feat: add branding

2a428e2

feat: implement design system

8748cc5

chore: change brand logo file

06b6830

feat: add settings styles component

b014914

fix: improve initialization

c5d66d1

fix: fix minor bugs

05c111b

feat: add additional values

19ab800

fix: move branding toggle out of the styling toggle

c9d03e2

fix: refine dark style theme

5c105f3

fix: make branding dynamic

90e8734

fix: remove direct classlist add and leave only reactive use effect

bea51d2

fix: color initialization on theme

098d8d2

test: add tests

ddc9c7c

Merge branch 'main' into feat/add-custom-styling-and-branding-v2

b3d1e7e

tests: move test out of init clause

273ce38

anfibiacreativa mentioned this pull request Dec 5, 2023

[feat] create a new function for this.renderBrandingAvatar #157

Closed

shibbas and others added 2 commits December 5, 2023 20:19

fix: merge issues causing test failure

48d95c4

fix: fix layout cosmetics and remove avatar from bubbles

38a3601

shibbas reviewed Dec 6, 2023

View reviewed changes

anfibiacreativa added 12 commits December 6, 2023 09:17

fix: make link icon generic and remove BEM from styles

5bade00

fix: fix dark theme toggle

d581f8a

fix: remove boolean and icon specific bindings for chat avatar

d29ed06

fix: remove styles from chat-thread

f211a14

fix: remove duplicated styles in wrong context

784b0dd

fix: update color name

52d2526

chore: optimize default names

5cf0898

chore: update init object for colors styling

fdcc73b

fix: fix chat stage rendering conditional

ba18c36

test: fix enable branding test

9565223

test: fix theme switch test

5e1a5e1

test: fix broken follow up questions test

25498dc

anfibiacreativa marked this pull request as ready for review December 7, 2023 19:28

anfibiacreativa added 2 commits December 11, 2023 14:39

feat: change items to compliant name. Closes #163

9fdea58

test: fix tests to match new names

cc97d0c

manekinekko approved these changes Dec 11, 2023

View reviewed changes

sinedied reviewed Dec 11, 2023

View reviewed changes

anfibiacreativa mentioned this pull request Dec 11, 2023

[feat] override styles only through CSS vars #164

Closed

fix: remove unnecessary empty container in return

c61aaca

shibbas reviewed Dec 12, 2023

View reviewed changes

shibbas approved these changes Dec 12, 2023

View reviewed changes

anfibiacreativa added 2 commits December 12, 2023 13:33

fix: remove unnecessary css rule

f0525ce

Merge branch 'main' into feat/add-custom-styling-and-branding-v2

d0bf449

anfibiacreativa merged commit 703d278 into main Dec 12, 2023
6 checks passed

anfibiacreativa deleted the feat/add-custom-styling-and-branding-v2 branch January 19, 2024 14:51

		@@ -108,6 +119,26 @@ export class ChatComponent extends LitElement {

		static override styles = [chatStyle];

		override updated(changedProperties: Map<string \| number \| symbol, unknown>) {

feat: add custom styling and branding v2 #160

feat: add custom styling and branding v2 #160

Conversation

anfibiacreativa commented Dec 5, 2023

Purpose

Does this introduce a breaking change?

Pull Request Type

How to Test

What to Check

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shibbas Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manekinekko commented Dec 11, 2023

anfibiacreativa commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shibbas commented Dec 12, 2023

shibbas Dec 6, 2023 •

edited

Loading