devedmonton / DES-Website

The Dev Edmonton Society website! We empower Edmonton Developers!
https://devedmonton.com
MIT License
22 stars 52 forks source link

Event Calendar #346

Closed keifererikson closed 2 months ago

keifererikson commented 2 months ago

What issue is this referencing?

This is referencing #312. Currently a work in progress.

I was unsure about creating a new "Calendar" link at the top of the page, or if this should be added to the bottom/top of the "Events" page.

Any tips or feedback would be greatly appreciated!

image

Do these code changes work locally and have you tested that they fix the issue yourself?

Does the following command run without warnings or errors?

Have you taken a look at our contributing guidelines?

My node version matches the one suggested when running nvm use?

keifererikson commented 2 months ago

I am currently working on implementing the new API that @dgmouris added with #343.

However, I ran into this CORS error when attempting to fetch data from the API: Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://devedmonton.com/api/events. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing). Status code: 200.

@dgmouris is there a fix you can make on the server side for this?

dgmouris commented 2 months ago

@keifererikson this is awesome just a heads up in reference to.

However, I ran into this CORS error when attempting to fetch data from the API: Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://devedmonton.com/api/events. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing). Status code: 200.

Enabling CORS on this endpoint will be necessary, especially since setting up a testing environment for this is a bit of a pain and a bit unnecessary. I'll push something up shortly, unless folks have any qualms about it.

dgmouris commented 2 months ago

Just worked with @keifererikson to get his environment working locally on a test account to get around this CORS issue.

arashsheyda commented 2 months ago

@dgmouris I wonder if this (see the cors section) is any help for the cors issue, maybe we could try it

update: I test it and it works 🥳

dgmouris commented 2 months ago

@dgmouris I wonder if this (see the cors section) is any help for the cors issue, maybe we could try it

update: I test it and it works 🥳

Hey @arashsheyda I just took a look and it looks good to me so I just merged it so that @keifererikson can work with better data, I hope that's cool.

keifererikson commented 2 months ago

Awesome! Thanks for the quick fix on that one @arashsheyda and @dgmouris! It's working on my end now! 😄

MandyMeindersma commented 2 months ago

The code looks good to me! Thanks again @keifererikson for putting so much effort in!

I am so excited to have this on the site!

The deploy preview you should be able to see. You can click "details" on this line item:

image

It's not rendering for me so I am wondering what command you use to run locally? Netlify uses npm run build which then involves another command npm run start so I never use it while building locally but I am wondering if that's what is causing the rendering issues?

MandyMeindersma commented 2 months ago

Oh also, about the header, we can probably remove the "events" tab and squish it in with the meetup tab (or something) but that can be a later problem, like after you merge :)

Great work!

keifererikson commented 2 months ago

Hi @MandyMeindersma! It appears I am having some difficulty figuring out how Nuxt wants me to handle useFetch/useLazyFetch. (You'll have to forgive me, I'm still pretty green with all of this. 😳) It's not rending for me unless I refresh the page... which is interesting. I have some more work to do that's for sure!

I was going to actually ask your opinion on how you would like the header to be handled! We could put the calendar either above or below the events on the Events page, or like you mentioned, put the Event and Meetup pages together! I'm not sure what the optimal approach would be.

MandyMeindersma commented 2 months ago

Oh the refresh does work for me! It looks great. I love the dialog and everything.

I like your idea of adding the calendar to the events page! Add it at the top. :)

arashsheyda commented 2 months ago

@keifererikson this is a great work, thank you! a hint about Nuxt fetching is that Nuxt fetch works on ssr too, so you don't need to asign the fetched data to another ref

and for transforming the data you can use transform method

const { pending, data } = await useLazyFetch('https://devedmonton.com/api/events', {
transform: (data) => {...},
})
dgmouris commented 2 months ago

On refresh this works for me too, once that's fixed I'd be down to merge this and see what happens!

Awesome job @keifererikson :)

MandyMeindersma commented 2 months ago

Oh yeah! The only other weird thing that is happening for me (but idk it might not be happening for other people cause I am on a different time zone) there seems to be an off by one error on the days of the week? Like the month view is fine but when I go to the week view there are wednesdays events on thursdays and tuesdays event son wednesdays

image
keifererikson commented 2 months ago

@MandyMeindersma I noticed that as well! Turns out it's a known issue with the vue-cal package. (See the issue here: https://github.com/antoniandre/vue-cal/issues/505) There is a PR that resoles the issue, but has yet to be merged. For the meantime I can disable start-week-on-sunday as that will resolve the error for now.

arashsheyda commented 2 months ago

@keifererikson what do you think of refactoring the EventDetail component to something more general like Modal so if needed in the furure in other pages, we could use it

keifererikson commented 2 months ago

@arashsheyda I can definitely work on that! I'm pretty new to building components, so I definitely appreciate the feedback! I'm not sure how to make it super reusable.. I always run into the issue where what I'm working on requires specific data/inputs, etc. I'll do some research on best practices and see if I can improve it!

If you have any further tips I'm definitely open to hearing them too!

arashsheyda commented 2 months ago

@keifererikson no, you're doing great! I think it just need renaming the component and using a slot for content

About tips I'd say cehck VueUse, it's amazing :)

for example the current code for the modal (show, @close) is great and works! vueuse offer the same thing as a composables so we would save time in writing code

keifererikson commented 2 months ago

I think I've made some changes to make the Modal.vue component more reusable! (Using a slot and onClickOutside)

I've been struggling to get useVModels/useVModel to work though. I'm not sure if I understand it entirely. I've read through the docs but I keep hitting snags.

I'm not sure if I should be configuring it on the Calendar.vue component, or the Modal.vue component. I've tried both, but have been unsuccessful. If you have any further tips @arashsheyda on how to approach this that'd be amazing!

arashsheyda commented 2 months ago

@keifererikson thanks, looks great!

just one thing, we could use the footer part in modal and wrap it in name slots, it's not necessary though, jsut a thought:

<slot name="footer">
        <div class="flex items-center p-4 border-t border-gray-200 rounded-b dark:border-gray-600">
          <button
            type="button"
            class="duration-300 transition-all hover:bg-gray-200/30 dark:hover:bg-transparent border border-transparent rounded-lg bg-primary text-white px-3 py-1 hover:border-primary hover:text-primary flex items-center"
            @click="emit('close')"
          >
            Close
          </button>
        </div>
      </slot>

so the footer is always there by default and if we didn't want it or wanted to change the style, we can use <template #footer>...</template> in the AppModal


we don't have to use useVModel here, I just added that there for information. but if we were to use it would look like something this:

first we want to replace show-model & @close in Calendar.vue with v-model (I love v-model so much hahaha): <AppModal v-model="showEventModal">

you get more info about how it works here

then in the Modal.vue compoennt:

import { useVModel } from '@vueuse/core'

const props = defineProps<{
  modelValue: boolean
}>()

const modal = useVModel(props, 'modelValue')

const modalRef = ref()

onClickOutside(modalRef, () => {
  modal.value = false
})

v-model by default is modelValue, but you can use other things, for example if you wanted to have a open prop but as v-model, you would write it like this:

// Calendar.vue
<AppModal v-model:open="showEventModal">

// Modal.vue
const props = defineProps<{
  open: boolean
}>()

...other things

feel free to ask any question, I'd be happy to help! and sorry if I made you confused & didn't explain well 😄

keifererikson commented 2 months ago

Wow, I appreciate your time and help with all of this @arashsheyda!

That's a great idea with the Modal footer! I've made that change. 😄

What the heck, v-model is awesome! That makes it so much easier hahaha. I appreciate the tip on that one! So many cool things to learn that can make my life so much easier. LOL.

Thanks again @arashsheyda, you're a life saver!

arashsheyda commented 2 months ago

no problem, anytimw! thank you

MandyMeindersma commented 2 months ago

It looks SO GOOD!

I know there were a couple features you wanted to play around with @keifererikson but this is in a good enough state that I feel like we could merge it! Great work

keifererikson commented 2 months ago

I have this anxiety where it feels like it is never ready! Hahah. 🥲

But if you think it is at a releasable point I can mark the PR as ready for review! Thanks for all your help @MandyMeindersma @dgmouris and @arashsheyda !

dgmouris commented 2 months ago

@MandyMeindersma I agree with you and @keifererikson I think it's time to remove "WIP" from the title as this should definitely be merged, props to you on this hard work!

As well I definitely learned a few things here @arashsheyda thanks for this!

keifererikson commented 2 months ago

Ah thank you for the reminder on updating the title @dgmouris!

arashsheyda commented 2 months ago

great work everyone! and thanks

ajyong commented 2 months ago
image

I pressed the button, did some reading but honestly worried I may have caused bad consequences. It failed because it's using an old Node v18. Is this related to #336?

ajyong commented 2 months ago

WIP in the title, or a wip label are the "natural workarounds" I've seen out there, but none prevent accidental merges. I don't intend to single-out anyone (and GitHub is to blame for this product confusion: draft PRs are not free for private repos) but they've an official way to mark a PR as WIP: https://github.blog/2019-02-14-introducing-draft-pull-requests/

keifererikson commented 2 months ago

Hi @ajyong !

@arashsheyda may be more familiar with that Node issue than I would, unfortunately.

For the WIP comment, I'm not sure what you are meaning with it, but this was a draft PR originally! I changed it to open just a couple days ago. If I'm misunderstanding something please let me know!

arashsheyda commented 2 months ago

here is the error from the logs: The engine "node" is incompatible with this module. Expected version "^18.18.0 || ^20.9.0 || >=21.1.0". Got "18.17.0" I think 20.x would be good to use...

and to change the node version: https://github.com/devedmonton/DES-Website/blob/main/.nvmrc I was hoping someone would do it in the pr-athon but it's still open

also I saw in the deploy details it's using yarn instead of npm? (sorry I'm not familiar with netlify's building process either so I have no idea about this 😄 )

ajyong commented 2 months ago

For the WIP comment, I'm not sure what you are meaning with it, but this was a draft PR originally! I changed it to open just a couple days ago. If I'm misunderstanding something please let me know!

Oops, my apologies, I made an incorrect assumption. I thought GitHub shows when someone "un-drafts" a PR but I guess not!

keifererikson commented 2 months ago

No problem! It looks like this is the not-very-descriptive way github tells us. Hahah.

image

dgmouris commented 2 months ago

@ajyong I totally blanked and forgot about this when I suggested to @keifererikson my bad! Setting it as draft is way better.

WIP in the title, or a wip label are the "natural workarounds" I've seen out there, but none prevent accidental merges. I don't intend to single-out anyone (and GitHub is to blame for this product confusion: draft PRs are not free for private repos) but they've an official way to mark a PR as WIP: https://github.blog/2019-02-14-introducing-draft-pull-requests/

MandyMeindersma commented 2 months ago

349

Sorry 😢

MandyMeindersma commented 2 months ago

@allcontributors add @keifererikson for code and @arashsheyda for reviews

allcontributors[bot] commented 2 months ago

@MandyMeindersma

I've put up a pull request to add @keifererikson! :tada:

MandyMeindersma commented 2 months ago

@allcontributors add @keifererikson for code

allcontributors[bot] commented 2 months ago

@MandyMeindersma

@keifererikson already contributed before to code

MandyMeindersma commented 2 months ago

@allcontributors add @keifererikson for code

allcontributors[bot] commented 2 months ago

@MandyMeindersma

@keifererikson already contributed before to code

MandyMeindersma commented 2 months ago

@allcontributors add @arashsheyda for reviews

allcontributors[bot] commented 2 months ago

@MandyMeindersma

I've put up a pull request to add @arashsheyda! :tada: