feat: add custom styling feature #137
Conversation
anfibiacreativa
changed the title
for new feature
| await page.goto('/'); | ||
| await page.getByTestId('button__developer-settings').click(); | ||
| await page.locator('label').filter({ hasText: 'Select theme' }).click(); | ||
| await page.waitForTimeout(1000); |
There was a problem hiding this comment.
generally timeout is prone to flakiness. It's better to wait for an element state instead. You can increase the timeout on the element check instead. (same for all the waitTimeouts below)
There was a problem hiding this comment.
Agreed. We leave it also for the follow up optimization.
| await page.getByTestId('button__developer-settings').click(); | ||
| await page.locator('label').filter({ hasText: 'Select theme' }).click(); | ||
| await page.waitForTimeout(1000); | ||
| // check if the body has the class "dark" |
There was a problem hiding this comment.
you can use locators page.locator(".dark") instead.
There was a problem hiding this comment.
We are thinking about a task to improve all tests, once we have all merged in.
| }); | ||
|
|
||
| const [customStyles, setCustomStyles] = useState(() => { | ||
| const styleDefaultsLight = { |
There was a problem hiding this comment.
should these be in their own file for easy changing?
There was a problem hiding this comment.
Yes, agreed. Let's also put it in the overall optimization task.
| // Update the body class and html data-theme | ||
| localStorage.removeItem('customStyles'); | ||
| document.body.classList.toggle('dark', newIsDarkTheme); | ||
| document.documentElement.dataset.theme = newIsDarkTheme ? 'dark' : ''; |
There was a problem hiding this comment.
this should probably use reactive update of the React component rather than direct class add
There was a problem hiding this comment.
Let's put this in the same code optimization iteration
| // The following block is only necessary when you want to override the component from settings in the outside. | ||
| // Remove this block when not needed, considering that updated() is a LitElement lifecycle method | ||
| // that may be used by other components if you update this code. | ||
| override updated(changedProperties: Map<string | number | symbol, unknown>) { |
There was a problem hiding this comment.
check out and see if styleMap directive for lit would be useful here. https://lit.dev/docs/components/styles/#dynamic-classes-and-styles
| <header class="branding__banner"> | ||
| <a class="branding__link" href="${globalConfig.BRANDING_URL}" target="_blank" rel="noopener noreferrer"> | ||
| ${unsafeSVG(iconLogo)} | ||
| </a> |
There was a problem hiding this comment.
reuse this.renderBrandingAvatar
| @@ -694,7 +752,8 @@ export class ChatComponent extends LitElement { | |||
| <!-- Conditionally render default prompts based on hasDefaultPromptsEnabled --> | |||
| ${this.hasDefaultPromptsEnabled | |||
| ? html` | |||
| <div class="defaults__container"> | |||
| ${this.renderBrandingBanner()} | |||
| <div class="defaults__container" data-testid="chat-branding"> | |||
There was a problem hiding this comment.
this should probably be on the branding header and not the default container.
There was a problem hiding this comment.
I created a rendering function, so I'm not sure exactly what you mean here. But let's put it in the back burner to review when we have all merged in.
| @@ -10,6 +10,32 @@ export const enum RetrievalMode { | |||
| Text = 'text', | |||
| } | |||
|
|
|||
| export type CustomStylesState = { | |||
There was a problem hiding this comment.
this should probably not be in the api folder. Probably should be in components folder.
There was a problem hiding this comment.
Let's refactor for best practices once we have all old PRs in.
|
Will need to reimplement after refactoring has been integrated. |
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
The feature adds an improved