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

Add google calender button for meetings #4437

Merged
merged 9 commits into from Feb 19, 2024

Conversation

nilsvselte
Copy link
Contributor

@nilsvselte nilsvselte commented Feb 4, 2024

Description

Knapp på møter for å legge til møtet i Google Calender

Result

Knapp

  • Changes look good on both light and dark theme.
  • Changes look good with different viewports (mobile, tablet, etc.).
  • Changes look good with slower Internet connections.
Before After

Testing

Hadde bare muligheten til å teste med møter jeg selv er en del av, fungerer dog feilfritt med disse.
Løser
Frustrasjon over at det ikke var en knapp der.

@nilsvselte nilsvselte changed the title Add to google calender button for meetings Add google calender button for meetings Feb 4, 2024
@ivarnakken
Copy link
Member

Hi! 👋

Thanks a lot for contributing!

Although this is very useful in itself, it is actually something we already support. If you navigate to https://abakus.no/events and scroll all the way down (see image below) you are able to add both meetings and events to your calendar of liking. However, I can see how this feature is quite hidden from the user, so perhaps we can add these links to the meeting page as well? I think that would be super useful

In my opinion, the current "implementation" is more robust and "ideal" as it will automatically sync everything, so the user does not have to manually add every meeting to the calendar.

Thoughts?

image

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself looks mostly good, but let's figure out what we want before doing anything 😄

app/components/AddToCalender/AddToCalender.tsx Outdated Show resolved Hide resolved
Comment on lines 196 to 200
title={meeting.title}
startTime={meeting.startTime}
endTime={meeting.endTime}
description={meeting.description}
location={meeting.location}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's easier to only pass the meeting object as the prop and then destruct it in the AddToCalender component

Suggested change
title={meeting.title}
startTime={meeting.startTime}
endTime={meeting.endTime}
description={meeting.description}
location={meeting.location}
meeting={meeting}

Copy link
Contributor Author

@nilsvselte nilsvselte Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yes, is there a common ancestor for meetings and events? The idea was for it to be easily utilized in other scenarios aswell

app/routes/meetings/components/MeetingDetail.tsx Outdated Show resolved Hide resolved
target="_blank"
rel="noopener noreferrer"
>
<Button ghost>Legg til i Google Calender</Button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to be a nerd, but it's actually written Google Calendar 🤓👆

};

return (
<div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the <div> needed?

app/components/AddToCalender/AddToCalender.tsx Outdated Show resolved Hide resolved
@ivarnakken ivarnakken requested a review from a team February 4, 2024 12:55
@ivarnakken ivarnakken added discussion Pull requests that needs discussions, e.g. regarding controversial/big changes review-needed Pull requests that need review changes-requested Pull requests with requested changes labels Feb 4, 2024
@norbye
Copy link
Member

norbye commented Feb 4, 2024

I like it!

In my opinion there is no harm in implementing both the calendar sync and add a single event to calendar - as they might cater to different wishes.

And actually placing it inside the meeting details as Nils does is a genius move as that is where you are when you think of your meetings.

Proposed idea of the next iteration for this;

We can create a toggle, like MazeMap on the event pages, with something like "Legg til i kalender", which would open a section where the user can

  1. Add just this event to your calendar

  2. Synchronize all your meetings to your calendar automatically

  3. Synchronize all your meetings and events to your calendar automatically

This way it is in a much more visible place than it is right now, without taking up too much space and giving the possibility to explain the different options

@nilsvselte
Copy link
Contributor Author

I can make a draft for the toggle idea, and you guys can discuss it later if you decide it is appropriate:))

nilsvselte and others added 3 commits February 5, 2024 12:27
Co-authored-by: Ivar Nakken <69514187+ivarnakken@users.noreply.github.com>
Co-authored-by: Ivar Nakken <69514187+ivarnakken@users.noreply.github.com>
Co-authored-by: Ivar Nakken <69514187+ivarnakken@users.noreply.github.com>
@falbru
Copy link
Contributor

falbru commented Feb 5, 2024

Great job! 🔥

Although we already have a similar feature implemented, this is a very good reminder that it's too hidden for the average user. Moving it to the meeting view like you've done is a very good idea! If you could implement something similar to what @norbye proposed, that would be awesome!

@nilsvselte
Copy link
Contributor Author

I added a proposal for the toggle, I don't know the backend though and it seems like the current calendar options utelize the backend so I could not implement the other options. I can take a look at it when there is less school 😅

@norbye norbye force-pushed the GoogleCalenderButtonMeetings branch from 0b938a2 to e8f0283 Compare February 10, 2024 13:54
@norbye
Copy link
Member

norbye commented Feb 10, 2024

Changed the design slightly with a new suggested look, and moved it to under the map as I'd think you need the map more frequently than you need the export

Hidden Visible
image image
image image

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

Just some nitpick hehe

app/routes/meetings/components/MeetingDetail.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventFooter.tsx Outdated Show resolved Hide resolved
app/routes/events/components/EventFooter.tsx Outdated Show resolved Hide resolved
app/components/AddToCalendar/AddToCalendar.tsx Outdated Show resolved Hide resolved
app/components/AddToCalendar/AddToCalendar.css Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added the enhancement Pull requests that make enhancements, instead of just purely new features label Feb 12, 2024
@norbye norbye force-pushed the GoogleCalenderButtonMeetings branch from e8f0283 to e197e94 Compare February 17, 2024 20:30
@norbye
Copy link
Member

norbye commented Feb 17, 2024

Just some nitpick hehe

I would expect nothing else 💯

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ivarnakken ivarnakken added approved Pull requests that have been approved and removed changes-requested Pull requests with requested changes labels Feb 17, 2024
@norbye
Copy link
Member

norbye commented Feb 18, 2024

@nilsvselte I don't think you have permission to give approving reviews on this on github, but as it is your PR I'd love to hear it if you have any thoughts about the changes before I merge and deploy it:)

@nilsvselte
Copy link
Contributor Author

No, it looks great:) Learned a lot, thank you ❤️

Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@ivarnakken
Copy link
Member

Ran it through our CI now - no errors! 🎉 I will merge this and deploy it by the end of the day

Awesome job! Thanks again for contributing! 😄

@ivarnakken ivarnakken merged commit e467924 into webkom:master Feb 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved discussion Pull requests that needs discussions, e.g. regarding controversial/big changes enhancement Pull requests that make enhancements, instead of just purely new features review-needed Pull requests that need review
Projects
None yet
4 participants