-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Microsoft Calendar Plugin #16101
Microsoft Calendar Plugin #16101
Conversation
Changed Packages
|
Uffizzi Preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 thanks for this!
Looking pretty good code wise, just a few comments to clear up.
Would also be pretty good to write some test for these components that you've got too. I see a lot of test-id
being used, but no tests :(
@benjdlambert i have done changes according to feedback, are we good to integrate this. |
Would it be possible to write some tests for this? Just some simple ones that just render the components and make sure that the expected content is there? It's pretty hard for us to accept other contributions to this plugin when there's no test that ensure that the plugin still works :) |
Another separate question - where did you get the svg icons? Are they open source and licensed properly that we can copy the contents here? This change will also need a changeset too which you can generate using |
the icons are from https://www.svgrepo.com which provide open-licensed SVG vectors. i will add the changeset right away |
i have made changes in dev folder and ensured that there is a working demo available for every user, just one command away. test case's i will try to contribute in the future or will raise it as an issue so if someone is eager can pick it up early. @benjdlambert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool contribution, this is something we might add to our homepage in the future, many thanks @Abhay-soni-developer.
Just left some comments about the documentation
.changeset/rotten-birds-build.md
Outdated
'@backstage/plugin-microsoft-calendar': patch | ||
--- | ||
|
||
created new plugin @backstage/plugin-microsoft-calendar and added it to plugins directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be really great to include how to use it here as this changeset will get used as the release notes. For those upgrading theres a good chance this will be the way they learn about this plugin first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abhay-soni-developer this comment still stands I believe
plugins/microsoft-calendar/README.md
Outdated
@@ -0,0 +1,19 @@ | |||
# microsoft-calendar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably improve this a lot so that people won't run into issues using this. It would be great to have step by step instructions as to how to install - the yarn add
command for the package, where does this get added, etc.
Also, looking over the code it assumes Azure AD auth has been setup, it would be good to include information about that here even if it just a link to the main documentation on this topic. Without this information the calendar won't work and people will log issues about it or simply not use the plugin, which would be a shame as it's pretty awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example you can use to help for the installation instructions: https://github.com/backstage/backstage/tree/master/plugins/azure-devops
And here's the docs for the Azure auth: https://backstage.io/docs/auth/microsoft/provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will update readme accordingly and push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requested changes done @awanlin @benjdlambert are we good to integrate this?
Thanks for the contribution! |
fcb078a
to
48a0a14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Abhay-soni-developer! 👋
Some more comments here, but i think we're getting close!
.changeset/rotten-birds-build.md
Outdated
'@backstage/plugin-microsoft-calendar': patch | ||
--- | ||
|
||
created new plugin @backstage/plugin-microsoft-calendar and added it to plugins directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abhay-soni-developer this comment still stands I believe
.changeset/rotten-birds-build.md
Outdated
--- | ||
|
||
created new plugin @backstage/plugin-microsoft-calendar and added it to plugins directory | ||
updated example-app homepage to show microsoft-calendar widget. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed the changeset, if it should be anything else, please let me know and elaborate on what do we need to write here in case of a new plugin.
plugins/microsoft-calendar/src/components/CalendarEventPopoverContent.tsx
Outdated
Show resolved
Hide resolved
return response.json() as Promise<T>; | ||
} | ||
|
||
public async getCalendars(params?: any): Promise<MicrosoftCalendar[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we type params
better here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
params will do, they are query path param, or please suggest something better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the any
type. Can we type those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, removing this as we do not need any query paramters in this request
@benjdlambert i have done the changes that u asked for, please have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small nits, and you will need to rebase the branch in order to fix the merge conflict in the CODEOWNERS file.
This comment too: #16101 (comment)
plugins/microsoft-calendar/src/components/CalendarEventPopoverContent.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: Abhay-soni-developer <abhaysoni.developer@gmail.com>
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
bd148ba
to
f2beeee
Compare
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
Signed-off-by: blam <ben@blam.sh>
hey @benjdlambert , i can see that you have made the required changes, is there anything from my side left to do , or we are ready to merge this one. |
Thanks for the effort and the hard work! 🙏 |
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
@benjdlambert @awanlin for your support and appreciations. |
🔖 Summary
the request for this feature is here : #16001
the plugin will give a card component where the user can see his daily calendar schedule, and switch between different calendars, if a meeting URL is mentioned, the user can directly click on it to join a meeting and cancelled meetings are stroked.
hovering over any event will pop over a card showing you the list of people invited with a badge over each showing their responses.
microsoft-calendar-plugin-demo.mp4
✌️ Context
Creating a plugin to achieve the following feature:
green --> accepted
red --> declined
nothing --> not responded yet