Stampy redesign #735
Stampy redesign #735
Conversation
feedback form not showing
3 columns instead of 4
human answers
use human answers when available
Chat settings
Stampy redesign 591
fixed settings squish
Mobile issues fixed, bugs fixed, improved search
No more youtube authors, and autocomplete for search off.
mruwnik
requested review from
melissasamworth,
plexish,
Aprillion,
chriscanal,
LeMurphant,
ccstan99,
MatthewBaggins,
bryceerobertson and
zarSou9
app/assets/icons/StampySmall.svg
Outdated
There was a problem hiding this comment.
(minor) why do we have 2 sizes? SVG is supposed to be "scalable" vector graphics...
app/assets/icons/stampy.svg
Outdated
There was a problem hiding this comment.
the extra whitespace after changing viewBox is expected?
| .submit { | ||
| height: var(--spacing-48); | ||
| border-radius: var(--border-radius); | ||
| font-weight: 500 !important; |
There was a problem hiding this comment.
(minor)
a) does important help?
b) do we even use a font that has weight 500? (non-variable fonts usually have 400 = normal, 700 = bold, but not semi/demi-bold variations)
| @@ -0,0 +1,293 @@ | |||
| export const CHATBOT_URL = 'https://chat.stampy.ai:8443/chat' | |||
There was a problem hiding this comment.
this shouldn't come from environment variables?
There was a problem hiding this comment.
it should. But I gave up after trying various things and nothing working
| question?: string | ||
| } | ||
|
|
||
| export const formatCitations: (text: string) => string = (text) => { |
There was a problem hiding this comment.
the chat-related files are copied from the other repo and already tested, right? we didn't have any automated tests to be copied too, please? 👀
There was a problem hiding this comment.
no tests :/
and also could maybe do with more checking - I found quite an interesting bug there
| // don't ask | ||
| setActionTaken(actionTaken) |
There was a problem hiding this comment.
@zarSou9 explain yourself! For the allseeing gaze of Peter has fallen upon you!
There was a problem hiding this comment.
aaaaahhhhh, not the all-seeing gaze!!! I mean I did give some pretty explicit instructions for people not to ask, but here goes anyway. Basically, because react batches state changes when it wants to, actionTaken on line 167 does not see the change of 152 and thus I am setting action taken to its original state before the state change on line 152. Anyway, I've since learned that the correct solution to this is to pass a function to setState, like so: setState((v) => !v). This works for now, but I'll change it next time i'm working on the code :).
There was a problem hiding this comment.
I might have seen that problem once or twice... I think the code is fine for that purpose (using (v) => !v could be wrong if the handler is called multiple times), but the comment could improve:
| // don't ask | |
| setActionTaken(actionTaken) | |
| // if response is not ok, revert to original state as it was before this handler started | |
| setActionTaken(actionTaken) |
app/routes/questions.editors.tsx
Outdated
There was a problem hiding this comment.
why not deleting the file?
There was a problem hiding this comment.
another non-deleted file?
public/favicon-min-512.png
Outdated
There was a problem hiding this comment.
we deleted the code to switch between 2 favicons => this file shouldn't be re-added
public/favicon-min.ico
Outdated
There was a problem hiding this comment.
and this file is not needed either
Fix reference order
|
I think #643 is sufficiently problematic to be launch-blocking, since it seems to consistently happen at around the 3rd or 4th prompt. Other than that looks great. |
fix chrome bug
New chatbot UI deployment.
Testable version at https://redesign.aprillion.workers.dev/chat/