-
-
Notifications
You must be signed in to change notification settings - Fork 570
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 <FileTree>
component
#1308
Add <FileTree>
component
#1308
Conversation
🦋 Changeset detectedLatest commit: 9e881ec The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
* main: (69 commits) [i18nIgnore] docs: `pnpm install` → `pnpm add` (withastro#1324) [ci] format i18n(zh-cn): Update frontmatter.mdx (withastro#1362) [ci] format i18n(es): Update `index` (withastro#1360) [ci] format i18n(fr): Update index (withastro#1367) i18n(es): remove extra section (withastro#1370) i18n(ko-KR): update `index.mdx` (withastro#1363) [ci] format i18n(zh-cn): Update index.mdx (withastro#1361) docs(showcase): add OpenSaaS.sh (withastro#1359) feat(Testimonials): add testimonials to website (withastro#1104) [ci] format [ci] release (withastro#1332) fix: autogenerated sidebar alphabetical sort (withastro#1298) Avoid sidebar scrollbar hiding behind navbar (withastro#1353) Use spawnSync instead of execaSync in `git.ts` (withastro#1347) [ci] format [i18nIgnore] Add src alias (withastro#1322) ...
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.
Amazing work on this @HiDeoo as usual! Thanks for your patience waiting for a review. As you know, I’m very much in favour of adding this to our built-in components.
Before diving into the detail wanted to give some big picture feedback about some of the changes.
Adds a bunch of tests.
I love it! This was my big intended chunk of work when moving this into Starlight, so very happy to see it in place 🙌
All icons have been updated as some of them were outdated, just the first letter of the extension thrown in an SVG, etc. When possible, icons from the Unicons icon set have been used.
The existing <FileTree>
in docs uses the exact same Seti icon set as VS Code uses by default. I’m not 100% sure if we should move away from that.
Not so much because the alternative icons added in this PR are not nice, just because having an opinion has a cost, and it could be nice to say “We use the default VS Code icon set” and avoid discussion about whether icon X for file type Y is better than icon Z. (It also could make it easier to update when VS Code updates, although I didn’t make the process particularly automated when building out the initial icon set.)
I think filling obvious gaps makes sense, but not sure if we should deviate much more than that. (Although noting there’s still plenty of scope for regular maintenance when dealing with these. For example, since this PR was opened, Biome has already changed icon 😅)
What do you think? There may be more things to consider here, so I’m happy to discuss.
All icons have been made usable with the
<Icon>
component.
At first this made me nervous, but I think you’re right that this is sensible. (We might want to revisit the icon picker UI in the component docs in the future, but not required for this PR.)
I think the only thing to double check is if any of these icons could clash with names in the Unicons set? I might try adding all the Unicons in a separate PR to see if that helps us spot these things. If it is an issue, maybe we would come up with a prefix for file-type icons?
I'm not sure what is the best way to handle the removal of the old [docs
<FileTree>
component] to not trigger too much the i18n tracker
We do have special commit message syntax that allows us to opt certain files out, although Lunaria doesn’t document that yet. Nevertheless, I think your proposed strategy is good as it will help translators clearly see the important changes in this PR and easily ignore the follow-up one.
I think I am mostly fine with that. Altho, considering we plan to make these icons available in the
I think considering my previous point and this one, I should make a script to quickly fetch the Seti icons (from the repo used by VSC and not the JSON one which was not updated in 4 years, or maybe re-use the JSON one code, need to check), have an exclusion list, optimize them and then generate what we need in a temporary file or something.
Definitely, I think having at least official integrations, all package managers (so that #1453 is possible), etc. would be a small but nice improvement.
Totally agree, maybe even an Icons dedicated page listing at first the various places/components where you can use icons and then the icons themselves with a basic search input could be enough. I'll play around with that idea and see if I can come up with something nice.
I think I could bake that into the script I mentioned earlier so it's easier to maintain on the long run.
Perfect, I'll start preparing that PR when we are almost done with this one. I'll start thinking and working on that "icon refactor" but if you have any questions or suggestions, feel free to ask. |
* main: (126 commits) [ci] format [ci] release (withastro#1488) Fix rendering issue for text with unhandled Markdown directives (withastro#1489) i18n(es): update community-content (withastro#1493) i18n(es): update manual-setup (withastro#1492) Updates Korean UI translations (withastro#1487) i18n(fr): update `manual-setup` (withastro#1484) [ci] format i18n(ko-KR): update `manual-setup.mdx` (withastro#1482) i18n(ko-KR): update `community-content.mdx` (withastro#1483) [ci] format [ci] release (withastro#1481) [ci] format Make Starlight compatible with server output mode (withastro#1454) [i18nIgnore] community content: article description copy edit (withastro#1408) [ci] format i18n(it): Updated plugins.md and community-content.mdx (withastro#1480) i18n(fr): update `resources/community-content` (withastro#1479) [ci] format i18n(it): Modified everything in the /guides folder (withastro#1456) ...
Small update: played a bit with creating a script that generates definitions based on the most recent icons used by VSC, grab all the associated SVGs and optimize them and export everything to a generated file that we can import and use in Starlight. It supports overriding some icons with already existing built-in ones, and reports name conflicts. Altho, I just remembered at the end why I didn't do this before: some of their SVGs are huge and won't work as-is with already existing icons. This works fine in isolation by using the entire I personally don't know how to properly resize them programmatically (using a lightweight approach), I usually use Figma for that. I think there are 2 options here:
Any thoughts? |
* main: [ci] format [ci] release (withastro#1555) Update lockfile & patch version of EC to latest (withastro#1553) [ci] format [ci] release (withastro#1546) Improve Zod errors (withastro#1542) Update `astro-expressive-code` dependency to `^0.33.2` (withastro#1541) i18n(ja): Update pages.mdx (withastro#1551) i18n(es): update `plugins` (withastro#1548)
I guess there are a few options:
Good catch, we lost them when adding prefix support. This is now fixed in the generator and properly reflected in the generated file.
Wow, fun indeed ^^ I tried to repro on all my screens but didn't manage to. I wonder what would be the best approach to force that in order to fix it 🤔 I'll think more about it. |
Awesome — I think this should be pretty much good to go I guess. Will do a final pass through things now.
I’m fairly sure I did that too, although I don’t quite remember 😁 Of the options I guess we should either do nothing or use the official icon, even if we don’t like it. I’m fine with whatever you prefer. |
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 I thought — looks ready to go for the next minor! Just left one tiny suggestion.
Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.com>
Update the PR:
I think for this PR we should be good to go (except if we find any other issues ofc). The documentation follow up PR is available in #1563 |
Co-authored-by: Chris Swithinbank <357379+delucis@users.noreply.github.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.
Giving this an official stamp of approval for Friday’s release. Lovely work @HiDeoo! 🌟
* main: [ci] format [ci] release (withastro#1574) Add `<Steps>` component (withastro#1564) Add `<FileTree>` component (withastro#1308) Add icon support to the `<TabItem>` component (withastro#1568) [ci] format docs: add Flojoy to showcase (withastro#1571) i18n(es): update `components` (withastro#1547) i18n(pt-PT): add "manual-setup" page (withastro#1570) i18n(zh-cn): Update pages.mdx (withastro#1565) Updates internal github actions to the latest versions (withastro#1569) [ci] format i18n(it): Update pages.mdx & plugins.mdx (withastro#1567) [ci] format i18n(pt-PT): add "environmental-impact" page (withastro#1561) [ci] format i18n(zh-cn): Update plugins.mdx (withastro#1566)
What kind of changes does this PR include?
Description
This PR adds a
<FileTree>
component to display the structure of a directory.This pull request is a draft as some decisions still need to be made regarding various points described below and maybe discuss some implementation details.
Details
Rehype processor and component
Based on the version in the documentation, there is not a lot of changes, only minor ones:
Icons
<Icon>
component.argdown
,asm
,bsl
,cake
,cake_php
,code-search
,cu
,d
,go
(Slidesgo, not the language),grails
,hacklang
,jade
,makefile
,mdo
,odata
,pddl
,pddl-happenings
,pddl-plan
,prolog
,rescript
,slim
,smarty
,spring
,sublime
,twig
,wat
,wgt
.alpine
,biome
,cloudflare
,lit
,mdx
,netlify
,node
,pnpm
,preact
,solid
,tailwind
,vercel
.Regarding my train of thought for the icons, I imagined receiving feature requests for some icons being used in the file tree but not being available in Starlight so I made them all available. And if we decide to not do that, it's easier to remove icons vs. adding them all (I can attest to that ^^).
We may also decide to not include all the icons from my initial push, I just added everything possible based on the existing code and figured it would be easier to remove some of them if they are unwanted.
Docs
I added some documentation for the component and updated the english version of the docs site to use the new component altho I'm not sure what is the best way to handle the removal of the old one to not trigger too much the i18n tracker considering it is a drop-in replacement but this PR should still be tracked as it contains new documentation.
My current idea I have would be:
[i18nIgnore]
hast-util-from-html
,hast-util-to-string
,hastscript
,hast
,rehype
, andunist-util-visit
.Another approach could be to let translators handle the replacement of the component in their language and wait for them to do so before removing the old component and its dependencies.
If merged, the following tasks should be done before merging:
docs/src/content/docs/guides/file-tree-demo.mdx
demo file.