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.

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

Add update page title command #1004

Merged
merged 5 commits into from
Oct 3, 2024
Merged

Conversation

richard-to
Copy link
Collaborator

This change adds a command that can update the page title.

A few notes:

  • Turns out we set the page title after every state change, so a web component wouldn't have worked here
  • One issue I ran into design-wise is that we need to keep the state of the updated page title. Ideally, it seems like it sent with the UI state every time. In the end, I chose not to do that. Instead just stored this client-side state as a variable on Channel class for now, which I'm not sure is ideal. But I think can be refactored later if we get more uses cases where we have state that is separate from the main mesop state object. I guess we do have query params and it's possible I could have used a similar but simplified pattern for storing the page title.

Closes #331

Copy link
Collaborator

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

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

LGTM - a comment around the naming, otherwise looks good - thanks!

docs/api/commands/update-page-title.md Outdated Show resolved Hide resolved
mesop/tests/e2e/update_page_title_test.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

(no action needed) food for thought: it's a little weird to me the interaction between channel and shell (which has been a while for a long time) - they both want to call each other (which would be a cyclic dependency) so we use this indirection.

I think this PR doesn't add that much incremental complexity, but perhaps in the future we should refactor shell and channel. It seems like channel is doing too much (it's both the transport layer + a stateful rendering service) and could be split up into 2+ classes (although the exact split is kind of unclear to me now).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree about this. I'll keep this in mind for the future.

@richard-to richard-to merged commit c490a2e into google:main Oct 3, 2024
6 checks passed
@richard-to richard-to deleted the title-update branch October 26, 2024 23:53
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.

Dynamically set page title
2 participants