-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
(v5) Tutorial update following template change #10025
Comments
Ah! This is amazing! I've had my head down all day working in the files themselves, but this is all relevant in #10035 ! |
Maybe I should have mentioned you before (and maybe Chris and HiDeoo) but as I know you are busy I hesitated! 😉 |
Only for scripts! It's still a perfectly valid and encouraged pattern for styles! |
Let's do that entirely in a different issue/PR! |
Yes, let's tackle those here! |
1st & 2nd try re: INSTRUCTIONS: See my updates to the text / intro, and whether those are sufficient! 1st & 2nd try re: TYPESCRIPT ERRORS: YES! This is what we want, and still to do in the original PR. These should absolutely be tackled here. 1st & 2nd try re: Let's address this issue wherever it occurs in this PR, but I distinctly remember being told to ADD IN the await (which wasn't there in the |
https://github.com/withastro/astro/blob/next/examples/minimal/tsconfig.json |
Oh okay! I thought that applied to both.
Yes that makes sense, I just added it there so I wouldn't forget to talk about it. 😅
From what I've read, I think so! But I'm up for retesting the tutorial with these changes in mind and going all the way through this time. I can do this tomorrow, rather in the afternoon (for me), so within 24 hours I should be able to add more information.
Following the link, you are right. However, when I use
Maybe the |
Oh ok, so I followed the tutorial with Astro v4 apparently... I didn't think to check So I just tested both With And in both cases, I have the following prompt: I'm even more confused now. I thought the PR removed these questions... This definitely needs more testing... Edit: And even more confused since those questions was not present in my tests with v4... 👀 |
Uh, well... I'm not sure what to expect from all that! Might be time to ask the devs what's going on there! |
So I redid the tutorial, in full this time (except Github/Netlify). I relied on #10035. Other than what I've already noted, there's not much new. Typescript errors are just a little different (and again, the principle is the same: it doesn't know the shape of objects). Here, given the previous observation, I did not follow the steps to the letter but your modifications seem good to me.
Here my steps:
This is not really a change, rather a "bug". I'm not sure what to do here. Should we add a hint that if the changes are not taken into account we should try saving again? Typescript shows an issue with Step 1:
So we need to update the code snippet ( ```astro title="src/pages/index.astro" ins={2-6}
<Footer />
<script>
document.querySelector('.hamburger')?.addEventListener('click', () => {
document.querySelector('.nav-links')?.classList.toggle('expanded');
});
</script>
</body>
``` To be consistent we also need to update Step 2 in Importing a .js file: ```astro title="src/pages/index.astro" ins={7} del={3-5}
<Footer />
<script>
document.querySelector('.hamburger')?.addEventListener('click', () => {
document.querySelector('.nav-links')?.classList.toggle('expanded');
});
import "../scripts/menu.js";
</script>
</body>
``` Typescript shows an issue with Step 2:
It seems the error is slightly different with We also have this issue in:
Typescript shows a new error due to
Just a nit regarding the Step 4:
We miss an import ( We say to use If the idea is to show that there may be changes to be made, perhaps we could use: Typescript shows errors, but everything works! I noticed another difference between Erika's PR and |
Amazing, thank you @ArmandPhilippot !! My thoughts:
I'm a little confused because I don't see this previously on this page? This should be the step where As for any issues with what create astro now does vs what we thought it does/what the PR changed it to do, seems like maybe that should be an issue for the astro repo? |
Noting that there is an open PR #10035 and we would love help implementing the changes that Armand has laid out and I have responded to above! 🙌 |
The point 3 was easy to fix. However for 4 and 5... I'm not sure what is best. For {
frontmatter.tags.map((tag: string) => (
<p class="tag">
<a href={`/tags/${tag}`}>{tag}</a>
</p>
))
} But it doesn't seem to me that we've already defined the type of a variable before, so isn't that likely to be confusing for a beginner?
Each time we restart the Dev Server,
The alternative would be to explain that you need to open another terminal in VSCode, and run the And without this sentence, the
Indeed, on this page we do not use
---
const { frontmatter } = Astro.props;
---
<h1>{frontmatter.title}</h1>
<p>Published on: {frontmatter.pubDate.toString().slice(0,10)}</p>
<p>Written by {frontmatter.author}</p>
<slot /> If I'm not mistaken, the last modification of <p>{frontmatter.pubDate.toString().slice(0,10)}</p> So what we are proposing in Update any frontmatter values to match your schema does not seem to me to change anything. The code snippet tells us to insert Thanks to the content collection, Don't hesitate if I haven't managed to be clearer... 😅
Oh okay, I thought you might be able to get some internal info on this but an issue works too! |
Regarding I just checked the [ci] release PR and, I see that:
But I don't know how to proceed to test the beta version of Edit: Nevermind I finally understood how to combine both:
🎉 For the explanation, I thought Edit2: The styles still seems broken but I saw a new PR related to the |
Ooh, this didn't close with the merged PR, but we merged this! |
📋 Explain your issue
First, sorry if I choose the wrong template...
Incorrect Content Report
didn't seem to fit so...As discussed during Talking & Doc'ing yesterday (at least for me, the 14/11), the tutorial for v5 needs to be updated following the changes made by withastro/astro#12083.
I haven't finished the tutorial yet, it took me longer than I thought. (This is the first time I do it, you have to excuse me. 🎉 Well, it's mainly taking notes that takes time! 😄 )
I stopped at
. To avoid losing my notes, I preferred to share them now.First try with
pnpm create astro@beta
I tried this approach first but, I agree with Chris. If we keep
pnpm create astro@latest
, there are too many things to update. Even by modifying two steps while initializing the project, from step 12 it is no longer possible to follow the tutorial without major changes.Second try with
pnpm create astro@beta -- --template minimal
In summary, there are fewer changes but we will have problems with Typescript from Unit 5 onwards. I haven't looked for fixes yet, just noted the reported errors.
Also, I skipped the steps related to Github and Netlify because I don't see why they would be affected... and I didn't want to waste time with that. If needed, I'll redo the tutorial when I have more time.
We don't need
await
. In VS Code, this is underlined with three dots when we use it:'await' has no effect on the type of this expression.ts(80007)
. So it's probably best not to include it.Both
url
andfrontmatter
are underlined and show the following errorProperty 'url' does not exist on type 'unknown'.ts(2339)
.First, a nit: is there an Expressive Code syntax to highlight all the added
props
asins
? I had seen a post on Discord about this, and by going quickly, I also got caught! It's confusing that some lines are green while some parts, also added, are gray.Then, new Typescript error due to
frontmatter
infilteredPosts
:Property 'frontmatter' does not exist on type 'unknown'.ts(2339)
.First, a nit: same as before we don't need await for
const allPosts = Object.values(await import.meta.glob('../posts/*.md', { eager: true }));
Then, again, Typescript error due to
frontmatter
.I see two options:
Object.values
but this is not beginner friendlyEdit: I just checked and when using
--template minimal
, it seems the default Typescript preset is still"astro/tsconfigs/base"
. So I guess the Typescript errors I noted already exists in v4...Consistency
Then, in the next example and in the next page we show:
Shouldn't we keep
Published on:
to be consistent and avoid confusing the reader?The description was after the publication date in the previous pages, now it is before.
Since this is a check aimed at novices, it may be better to remain consistent... (and point 1 also applies here).
Accessibility, unrelated to v5
I think we have an accessibility problem in "Test your knowledge". Firefox developer tools report me a contrast issue for:
More or less related, I would say that the green color for the checkmark stings the eyes a little... especially for the light theme. (I don't know if you say that in English... "piquer les yeux", let's just say it's not pleasant to look at 😅 )
define:vars
, more or less related to v5 (v6?)I thought there was some discussion about
define:vars
. Both for scripts and styles. I haven't seen any PR related to this topic, so I guess that there is no progress on this question. I preferred to note it anyway. If its use is discouraged, maybe it's time to not display it in the tutorial. Or maybe this can wait until a more precise decision...On Style your About page, we use
define:vars
to define a CSS variables. Should this be updated?The text was updated successfully, but these errors were encountered: