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

Make app title configurable #1264

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

fuenfundachtzig
Copy link
Contributor

@fuenfundachtzig fuenfundachtzig commented Apr 29, 2024

This PR adds a field in the configuration that allows the user to change the title (HTML <title>) displayed in the browser.

Copy link

vercel bot commented Apr 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 30, 2024 10:01am
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 30, 2024 10:01am

@@ -81,6 +83,28 @@ export const AppConfigForm: React.FC = () => {
</FormItem>
)}
/>
<FormField
control={form.control}
name="apptitle"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we change the field to just title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found just "title" a bit ambiguous? But I am happy to change this if you prefer title. (Any alternative suggestions also welcome, of course.)

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the late response - maybe app_title then, instead of without a space

Copy link
Contributor

Choose a reason for hiding this comment

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

after that, i think everything looks good! thanks for the awesome contribution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the late response - maybe app_title then, instead of without a space

Sounds good, I will change to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after that, i think everything looks good! thanks for the awesome contribution

Thanks :)

@@ -55,6 +55,7 @@ class _AppConfig:
"""

width: Literal["normal", "medium", "full"] = "normal"
apptitle: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add some tests to test_codegen to validate parsing from py and writing to py

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be codegen.generate_filecontents and codegen.get_app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some tests. (Doing so I found there is already quite a large number of tests 😁)

Copy link
Contributor

@mscolnick mscolnick left a comment

Choose a reason for hiding this comment

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

thanks for the awesome contrib @fuenfundachtzig! 🚀

@mscolnick mscolnick merged commit 5886681 into marimo-team:main Apr 30, 2024
26 checks passed
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.4.8-dev5

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.

None yet

2 participants