Skip to content
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

Event Calendar #346

Merged
merged 38 commits into from
Jun 17, 2024
Merged

Event Calendar #346

merged 38 commits into from
Jun 17, 2024

Conversation

keifererikson
Copy link
Contributor

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?

  • yes! (For the most part, still a WIP. 😄 )`

Does the following command run without warnings or errors?

  • yes! npm run pr-checks

Have you taken a look at our contributing guidelines?

  • yes!

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

  • yes!

@keifererikson
Copy link
Contributor Author

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
Copy link
Collaborator

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

rawData is a temporary solution until we resolve the CORS error
@dgmouris
Copy link
Collaborator

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

@arashsheyda
Copy link
Collaborator

arashsheyda commented May 23, 2024

@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
Copy link
Collaborator

dgmouris commented May 23, 2024

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

@dgmouris dgmouris mentioned this pull request May 23, 2024
4 tasks
@keifererikson
Copy link
Contributor Author

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

@keifererikson keifererikson changed the title WIP: Event Calendar Event Calendar Jun 5, 2024
@keifererikson
Copy link
Contributor Author

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

@arashsheyda
Copy link
Collaborator

great work everyone! and thanks

@ajyong
Copy link
Member

ajyong commented Jun 7, 2024

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
Copy link
Member

ajyong commented Jun 7, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Member

ajyong commented Jun 7, 2024

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
Copy link
Contributor Author

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

image

@dgmouris
Copy link
Collaborator

dgmouris commented Jun 7, 2024

@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
Copy link
Contributor

#349

Sorry 😢

@MandyMeindersma MandyMeindersma merged commit 6970843 into devedmonton:main Jun 17, 2024
4 checks passed
@MandyMeindersma
Copy link
Contributor

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

Copy link
Contributor

@MandyMeindersma

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

@MandyMeindersma
Copy link
Contributor

@allcontributors add @keifererikson for code

Copy link
Contributor

@MandyMeindersma

@keifererikson already contributed before to code

@MandyMeindersma
Copy link
Contributor

@allcontributors add @keifererikson for code

Copy link
Contributor

@MandyMeindersma

@keifererikson already contributed before to code

@MandyMeindersma
Copy link
Contributor

@allcontributors add @arashsheyda for reviews

Copy link
Contributor

@MandyMeindersma

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

@keifererikson keifererikson deleted the calendar branch June 17, 2024 03:14
@keifererikson keifererikson restored the calendar branch June 17, 2024 03:14
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.

5 participants