Skip to content
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.

Sign up for GitHub

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

Jump to bottom

REDBOX-440 - Editable chat title #770

Merged
merged 21 commits into from
Jul 16, 2024
Merged

REDBOX-440 - Editable chat title #770

merged 21 commits into from
Jul 16, 2024

Conversation

brunns
Copy link
Contributor

@brunns brunns commented Jul 11, 2024 •
edited
Loading

Context

Changes proposed in this pull request

Guidance to review

Relevant links

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

@brunns brunns force-pushed the feature/REDBOX-440-editable-chat-title branch from 6564a03 to f4e2222 Compare July 12, 2024 08:53
@brunns brunns force-pushed the feature/REDBOX-440-editable-chat-title branch from fb9f2aa to b55caf2 Compare July 12, 2024 13:52
@brunns brunns marked this pull request as ready for review July 12, 2024 13:54
@brunns brunns marked this pull request as draft July 12, 2024 14:28
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 15, 2024
View reviewed changes
django_app/frontend/src/js/chats/streaming.js Outdated
console.log(`updating chat title to "${newTitle}"`);
this.send(newTitle)
this.dataset.title = newTitle;
this.heading.innerHTML = `${newTitle} ${this.pencilIcon}`;

Check warning

Code scanning / CodeQL

DOM text reinterpreted as HTML Medium

DOM text is reinterpreted as HTML without escaping meta-characters.
brunns and others added 15 commits July 16, 2024 11:38
@brunns
Use setting for length of title field.
9c10ca9
@brunns
Use an h2 for the heading, and replace existing hidden one rather tha…
a6ed9dc 
…n having both.
@brunns
Take advantage of Python 3.12 HTTPStatus improvements.
500ddd2
@brunns
Enter, escape, and focus loss all seem to be working now.
74c39f2
@brunns
Some rudimentary styling.
5bede02
@brunns
Add pencil icon.
9a7f715
@brunns
Add chat renaming to journey test.
a02118d
@brunns
Do a11y check on navigation to titled chat.
3e0512a
@brunns
Thanks to Kevin, seems to work now.
5b6904d
@brunns
Rename the show/hide element functions.
fd993bf
@brunns
Perhaps fix "DOM text reinterpreted as HTML" warning?
25c8d8a
@brunns
Show chat title for new chats as soon as we can.
967b6ca
@KevinEtchells @brunns
Update accessibility, progressive enhancement, and types
cd5d963
@KevinEtchells @brunns
Add styling to chat-title component
7a053e0
@KevinEtchells @brunns
Update integration tests following chat-title updates
79b9eb2
@brunns brunns force-pushed the feature/REDBOX-440-editable-chat-title branch from bd894dc to 79b9eb2 Compare July 16, 2024 10:44
rachaelcodes
rachaelcodes reviewed Jul 16, 2024
View reviewed changes
tests/test_journey.py
chats_page = chats_page.navigate_to_titled_chat("What architecture is in use?")
chats_page.chat_title = "About tech stuff."
chats_page = chats_page.start_new_chat()
chats_page = chats_page.navigate_to_titled_chat("About tech stuff.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the tests - it's v clear what you've changed

rachaelcodes
rachaelcodes reviewed Jul 16, 2024
View reviewed changes
django_app/frontend/src/js/chats/streaming.js Outdated
@@ -297,3 +307,125 @@ class DocumentSelector extends HTMLElement {
}
}
customElements.define("document-selector", DocumentSelector);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we want to start thinking about breaking this file up into its different components?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not a bad idea at all. The feedback component is already in a file of its own.

Copy link
Contributor Author

@brunns brunns Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea, and now we have Parcel doing the bundling this should be relatively clean and simple.

rachaelcodes
rachaelcodes reviewed Jul 16, 2024
View reviewed changes
django_app/frontend/src/js/chats/streaming.js Outdated

update = () => {
const newTitle = this.input?.value;
console.log(`updating chat title to "${newTitle}"`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we keeping this for the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - I'll get rid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone.

rachaelcodes
rachaelcodes approved these changes Jul 16, 2024
View reviewed changes
Copy link
Contributor

@rachaelcodes rachaelcodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
(I think CHAT_TITLE_LENGTH = 30 is a bit short, but it looks like that was a previous PR/decision.)

@brunns brunns force-pushed the feature/REDBOX-440-editable-chat-title branch from e19fa62 to b2bea2a Compare July 16, 2024 12:34
@brunns brunns merged commit 545c227 into main Jul 16, 2024
10 checks passed
@brunns brunns deleted the feature/REDBOX-440-editable-chat-title branch July 16, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinEtchells KevinEtchells KevinEtchells left review comments

@rachaelcodes rachaelcodes rachaelcodes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brunns @KevinEtchells @rachaelcodes