Closed arashsheyda closed 4 months ago
the board recruitment page seems to 404 https://deploy-preview-315--dev-edmonton.netlify.app/board_recruiting
Okay general thoughts:
@MarkBennett You do not have to review but I think you would find this pr really fun. The resdesign is so cool!
Okay general thoughts:
- This is beautiful and I am so excited. Thank you for all the work you have put into it.
- How do we deal with the TODO's? Is your plan to make issues for them and fix later or fix before the pr merges in?
- Most important question: when I am confused about something with nuxt content in the next 6 months, can I DM you hahahah
@arashsheyda: I really love how good the redesigned landing page looks. Great job on the redesign and the improved light house scores.
@MandyMeindersma: I was literally thinking the same thing you mentioned in #3. It would be nice to have some support if we end up having issues later on. Also, an FYI, I am helping my brother move today, so I can't review the PR until later this evening.
@MarkBennett You do not have to review but I think you would find this pr really fun. The resdesign is so cool!
Thanks for the heads up @MandyMeindersma! I'm just heading out for a spring break trip with the family but I might lurk on this just to see. Very cool and great work @arashsheyda! 😄
It has a dark theme! So cool @arashsheyda! 🤯
@MandyMeindersma
- How do we deal with the TODO's? Is your plan to make issues for them and fix later or fix before the pr merges in?
yes, the TODO's are more like nice to have things in the future, except contact page
- Most important question: when I am confused about something with nuxt content in the next 6 months can I DM you hahahah
yes for sure, anytime! I'de be super happy to help with anything related to vue & nuxt :DD
@MarkBennett @deejayjay thanks!
I'm not going to go through a whole review due to the quantity of changes, but I do have a few comments after a quick glance.
Great work on these changes by the way.
use pnpm instead of npm (as it is much faster)
I don't necessarily agree with this change. At the very least I think it should be separated into it's own PR.
test
and format
npm commands have been removed, and also removed from the PR guidelines.I could maybe do a more in depth look later, but these are the things that jumped out for me right away that I thought might merit some discussion.
I don't necessarily agree with this change:
pnpm
here is why we sohuld use pnpm
pnpm vs npm
also in my experience pnpm
works better in many ways but if you want we can roll back to npm
😄
At the very least I think it should be separated into it's own PR.
I thought including it here made sense because of all the changes. But if you think it's better separate, we can do that
It looks like our
test
andformat
npm commands have been removed, and also removed from the PR guidelines.
format
comamnd has been replace with lint
(prettier is good but sometimes it can be too strict, and I think we can have what prettier has with eslint)
also there is this article about eslint & prettier
It looks like our test setup has been removed entirely. That's a pretty big regression.
the tests
were removed as it had 3 basic tests, and I thought starting fresh will help us build better tests later on.
I could maybe do a more in depth look later, but these are the things that jumped out for me right away that I thought might merit some discussion.
thanks @DerrykBoyd
I thought including it here made sense because of all the changes.
It's definitely best practice to keep PRs as small as possible and single focus. Otherwise things can get stuck in limbo since now you end up with multiple discussions in the PR trying to work through opinions on each change.
That's an opinionated change, my main objection here is it's another small hurdle for contributors. But I'm not totally against it.
Here are the recommendations from the typescript-eslint
project on this topic.
Again, this isn't something I would get behind, but I don't mind if others want to go ahead with it.
Yes we didn't have many tests, but what we did have was a working test setup, and that's valuable. Before if any contributor wanted to add a test all they had to do was create a x.test.js
file and write the test, everything else was handled including running the tests on CI.
Having personally volunteered time to get that setup up and running, it stings a little bit to see it removed without replacement.
Again, overall I don't want these things to detract from the work done here. It's incredible that you volunteered this much time to improve the site! And if this was merged as is, I would still be incredibly pleased.
Suggested action items before merge for me:
Edited - I didn't see the README was already changed in all the files lol
@arashsheyda please don't revert the prettier change 😄 I think if others are ok with it it would be good to try the eslint only setup and see how that goes.
It might help with some issues we had recently with contributors editing files directly in github
Thanks for doing those other changes! I'm glad the tests are back
Excellent work again @arashsheyda
Feel free to open up a follow up PR for switching to pnpm
A couple odd behaviours around scrolling that I noticed in the deploy preview
https://github.com/devedmonton/DES-Website/assets/24216368/ea11307a-7a43-4905-8ab9-f5b9dbaf6256
https://github.com/devedmonton/DES-Website/assets/24216368/eb3ab84e-99c7-457d-bd6f-c55f95662436
@DerrykBoyd the navigation thing is because we are getting the content of that page from content directory (@nuxt/content
) and that requires a api fetch, we can make the fetch lazy (so the page would load instantly but we have to show some skeleton UI to user while the data loads) or we can prefetch the data (but it might effect the site's performance, edited: the pages are loading faster now with prefetch enabled, the performance is not effected that much) let me know what way you want to approach
yes I'm aware of that 😄 I don't have illustrator at the moment but when we convert the illustrations to svg, we can set their size easier
that must've been a bad build from netlify (it was working before and it works locally)
@DerrykBoyd the navigation thing is because we are getting the content of that page from content directory (
@nuxt/content
) and that requires a api fetch, we can make the fetch lazy (so the page would load instantly but we have to show some skeleton UI to user while the data loads) or we can prefetch the data (but it might effect the site's performance, edited: the pages are loading faster now with prefetch enabled, the performance is not effected that much) let me know what way you want to approach
Ideally the solution would be that the scroll on the page doesn't change while waiting for that transition. But I don't think this is a major problem, could be a follow up issue for someone to look into.
I don't have illustrator at the moment but when we convert the illustrations to svg, we can set their size easier
All good 😄 again, minor issue
must've been a bad build from netlify
Possibly, I'm not convinced though. Probably ok to see after final deploy if it's working. What I did notice is that image is not using our image-kit setup while the others are. Also the image in production points directly to unsplash, where the one on the deploy preview seems to be processed by Netlify now.
At this point I'll defer to @MandyMeindersma to get this over the line.
Thanks for you patience with those comments @arashsheyda much appreciated.
@DerrykBoyd
I modified the router to wait for page to fully load and then scroll to top, should I push it?
https://github.com/devedmonton/DES-Website/assets/38922203/bc2b0941-bd12-4557-a7de-dcbdfce83241
yes the link to image is from unsplash, and maybe netlify is caching it (if that happened in production as well, maybe we can upload it to imagekit)
I modified the router to wait for page to fully load and then scroll to top, should I push it?
Sure, that looks much better 😀
So I think in one of the recent changes we lost the "randomization" of the meetups and community events.
Like if a refresh the highlighted 5 tiles seem to stay the same?
I have a couple more comments but they will probably have to wait until Thursday! Sorry to make you wait @arashsheyda
Thank you again!
@MandyMeindersma we have the same code built on vercel and it's working fine (randomization and the unsplash image https://devedmonton.vercel.app/)
new assets in imagekit:
Also unsplash seems to not work on your site so I just created an imagekit for that too:
Again amazing work @arashsheyda
Other than answering my random nuxt context questions I think there are only 2 things I am semi blocking this pr for:
server/api/contact.post.ts
file @MandyMeindersma the randomize
function is in /utils/index.ts
@MandyMeindersma in order for randomize to work we need SSR. (because we are using server components, I can change it to work only in client-side and we wouldn't need SSR but that's not good for SEO as the items wouldn't show in server-side)
can we make sure the build command is build
?
netlify | vercel |
---|---|
@all-contributors add @arashsheyda for code and ideas
@MandyMeindersma
I've put up a pull request to add @arashsheyda! :tada:
What issue is this referencing?
this PR will closes #311, #139 (with breaking changes)
Description
use
pnpm
instead ofnpm
(as it is much faster)latest
nuxt
with@nuxt/devtools
support@nuxt/content
for writing static content in markdwon@nuxt/eslint
for eslint setup with new flat configuration@nuxtjs/seo
for SEO tools likeogImage
,SchemaOrg
,LinkChecker
and ...new component structure
nuxt-icon
for iconify iconsmoved the data from
store
toconstants
(with auto-import enabled now). maybe we can move it tocomposables
in the future.improved lighthouse score: (the score is improved but the performance might not always be at 100, jsut a little heads up)