Skip to content

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

Merged
merged 9 commits into from
Jun 18, 2024
Merged

feat: make the logo link configurable #68

merged 9 commits into from
Jun 18, 2024

Conversation

Nemikolh
Copy link
Member

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:

---
type: tutorial
logoLink: https://example.com
---

This PR also default the collection schema to strict() so that unknown properties are caught.

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

changeset-bot bot commented Jun 12, 2024

⚠️ No Changeset found

Latest commit: 17bf0bf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Nemikolh Nemikolh requested review from d3lm and SamVerschueren June 12, 2024 13:21
Copy link
Contributor

@SamVerschueren SamVerschueren left a 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(),
Copy link
Contributor

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...

Copy link
Member Author

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/....

Copy link
Contributor

@d3lm d3lm left a 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.

@Nemikolh
Copy link
Member Author

Very good point!

On the note of having a tutorialkit.config.ts, what do you think about having the options be provided directly to the astro integrations, (tutorialkit({ ... })) instead ?

This way end users can decide to have a separate file or not and it's one less file for end users.

@SamVerschueren
Copy link
Contributor

The reason why I would be more in favour of a tutorialkit.config.ts is because it's then separated from Astro and the underlying tech. I mean, we could support both though 🤷 . Also given we hide the astro.config.ts by default... It would also make configuring the github importer easier because you don't even have an astro.config.ts.

@Nemikolh
Copy link
Member Author

The reason why I would be more in favour of a tutorialkit.config.ts is because it's then separated from Astro and the underlying tech.

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.

It would also make configuring the github importer easier because you don't even have an astro.config.ts.

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 astro.config.ts anymore. The main reasons being:

  • To let users bring their own integrations
  • It's not a security issue, the whole thing is: components are executed server side. And if we have a tutorialkit.config.ts then it's the same anyway.
  • It one less build mode to support

Curious to hear your thoughts on how you envision the github importer.

Copy link
Member Author

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.

@Nemikolh Nemikolh requested review from d3lm and SamVerschueren June 14, 2024 13:44
@@ -3,6 +3,7 @@ import { webcontainerSchema } from './common.js';

export const tutorialSchema = webcontainerSchema.extend({
type: z.literal('tutorial'),
logoLink: z.string().optional(),
Copy link
Contributor

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.

Copy link
Member Author

@Nemikolh Nemikolh Jun 14, 2024

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 the logoLink 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 having logoLink in a config to also apply to editor.

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 instance
  • defaultRoutes which when set to false let Astro lookup routes in src/pages useful when you want full customizablility.

If possible I'd like to keep it this way. YMMV.

@Nemikolh Nemikolh requested review from d3lm and SamVerschueren June 18, 2024 15:24
@Nemikolh Nemikolh merged commit 2da64ae into main Jun 18, 2024
6 checks passed
@Nemikolh Nemikolh deleted the joan/logo-link branch June 18, 2024 15:45
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.

3 participants