-
Notifications
You must be signed in to change notification settings - Fork 85
feat: make the logo link configurable #68
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
Conversation
…s are mentioned by default
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @Nemikolh 🙏 .
@@ -3,6 +3,7 @@ import { webcontainerSchema } from './common.js'; | |||
|
|||
export const tutorialSchema = webcontainerSchema.extend({ | |||
type: z.literal('tutorial'), | |||
logoLink: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the logoLink
name. Should we just call it homepage
or home
instead? I'm fine with keeping it though. You know, naming...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I found logoLink
to be more explicit about what that property does but it doesn't look great.
I don't know what to think about homepage
or home
because you can have it point to anything like https://buymeacoffee.com/...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it that the logo link is configurable. One thing I was wondering if we should not use the meta file inside the content folder and instead have various tutorial wide things which are less related to the actual tutorial content be configured through a config file like tutorialkit.config.ts
which exports a POJO. That would also be more flexible and in terms of DX it's a little more convenient to work with than yaml. Also thinking about the top bar config etc. and maybe folks wanted to do some dynamic stuff and read data from other files? Just thinking out loud.
Very good point! On the note of having a This way end users can decide to have a separate file or not and it's one less file for end users. |
The reason why I would be more in favour of a |
AFAIK, the default TutorialKit experience is only ever going to support Astro and I don't see us supporting alternative techs. If we embraced it entirely, then we can also benefit from the full Astro ecosystem where users can bring any Astro integrations that they want. Lastly, we want to keep the Astro config around for the Astro VSCode extension. So this is a file that we can't really remove.
I've been changing my mind on that one recently, I don't think anymore that we should assume the branch does not have an
Curious to hear your thoughts on how you envision the github importer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per our discussion with @SamVerschueren, instead of relying on currentColor
we have the color as part of the logo (as this should often be the case for logos) and we lookup for a logo-dark
variant that if not found we fallback to logo
.
@@ -3,6 +3,7 @@ import { webcontainerSchema } from './common.js'; | |||
|
|||
export const tutorialSchema = webcontainerSchema.extend({ | |||
type: z.literal('tutorial'), | |||
logoLink: z.string().optional(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we now decide whether to use the TK integration for configuration or a separate file? Personally I'd prefer if we decided that and maybe added support for that in this PR before we go back and forth on frontmatter configurations? I mean it's always a little cumbersome for users if they have to update and if we have folks that use the frontmatter and we switch it to a config file very soon then updating isn't as smooth even if it's a small change but if we can avoid it that'd be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't decided on anything yet so I haven't changed it.
My opinion? I think that property should be in a frontmatter rather than in the configuration of the TK integration or a separate configuration file.
Here's why I think so:
- Having it in the frontmatter instead of a configuration file means that there's no need for the vite server to be restarted. Which makes sense to me given I'm only setting an href on an element in the UI.
- In the last sync meeting, we've been discussing of having a
TopBar.astro
(or equivalent) that could be picked up automatically to give full control over how the top bar is configured. This would not require any configuration change, so having one for thelogoLink
is weird to me. - We have
editor: false
which is also a frontmatter setting right now and also a UI concern so I see arguments of havinglogoLink
in a config to also apply toeditor
.
Lastly, I would keep configuration at the root of the repo to be focused on stuff like port, headers, routes, authentication. Today we have:
enterprise
to point at a StackBlitz Enterprise instancedefaultRoutes
which when set to false let Astro lookup routes insrc/pages
useful when you want full customizablility.
If possible I'd like to keep it this way. YMMV.
This PR makes the logo link configurable so that it can point anywhere.
To use this feature add the following property to your
tutorial/meta.md
file: