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

React webpart to manage bookmarks #5386

Merged
merged 12 commits into from
Mar 10, 2025
Merged

React webpart to manage bookmarks #5386

merged 12 commits into from
Mar 10, 2025

Conversation

sudeepghatak
Copy link

By submitting this pull request, you agree to the contribution guidelines

If you aren't familiar with how to contribute to open-source repositories using GitHub, or if you find the instructions on this page confusing, sign up for one of our Sharing is Caring events. It's completely free, and we'll guide you through the process.

To submit a pull request with multiple authors, make sure that at least one commit is a co-authored commit by adding a Co-authored-by: trailer to the commit's message. E.g.: Co-authored-by: name <[email protected]>

Put an x in all the items that apply ([x], no spaces between the [, the x, and the ] ), make notes next to any that haven't been addressed.

  • New sample
  • Bug fix/update
  • Related issues: fixes #X, partially #Y, mentioned in #Z

What's in this Pull Request?

The 'Bookmarks Manager' web part allows users to manage and organize bookmarks in a SharePoint list. Users can add, edit, and delete bookmarks, which are displayed in a grid layout. The web part supports both light and dark themes and integrates seamlessly with SharePoint.

Node Version

Node version used: 18.20.6

Checklist

  • [ X] My pull request affects only ONE sample.
  • [X ] My sample builds without any warnings
  • [ X] I have updated the README.md file's Version history. For new samples, created a new README.md file matching this template
  • [ X] My README.md has at least one static high-resolution screenshot (i.e. not a GIF) located in the assets folder.
  • [ X] My README.md contains complete setup instructions, including pre-requisites and permissions required
  • [ X] My solution includes a .nvmrc file indicating the version of Node.js

@Adam-it Adam-it self-assigned this Mar 9, 2025
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@sudeepghatak awesome work 👏 and sorry for the hold up 🙏
I managed to build and run the sample locally
I left a few minor comments I will try to fixup when merging

Copy link
Member

Choose a reason for hiding this comment

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

we should not update any file outside of the sample folder.
Lets revet this change

package.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

we should not update any file outside of the sample folder.
Lets revet this change


**THIS CODE IS PROVIDED *AS IS* WITHOUT WARRANTY OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING ANY IMPLIED WARRANTIES OF FITNESS FOR A PARTICULAR PURPOSE, MERCHANTABILITY, OR NON-INFRINGEMENT.**


Copy link
Member

Choose a reason for hiding this comment

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

the image at the end of the readme template is also needed here. We call it a visitor stats image and you may read about it in the contribution guide. It is needed for telemetry reasons. Lets add it

@@ -0,0 +1,120 @@
# Using React Accordion plugin with SPFx
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 this is the wrong title

@Adam-it
Copy link
Member

Adam-it commented Mar 9, 2025

@sudeepghatak unfortunately I cannot perform the fixup for you as your commits are wrongly made directly to the main brand of your forked repo.
When contributing a sample we should create a dev (any name is ok) branch and commit our changes there and then open a PR from the dev branch to this repo main branch, we should avoid doing the changes directly to main as this then blocks maintainers from any possible fixup. So when being on main branch in order to create a new branch for you changes you should run something like git checkout -b your-sample-name main.

Please try to move your changes to a separate branch (you may use the git cherry-pick) command to do that, and close this PR and open a new one. Along the way you may also apply the review comments I left in this PR. If you encounter any trouble along the way please do not hesitate to contact me and we may work it out together 👍
Thanks for contributing to PnP Community. You Rock 🤩

Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

We need to move current changes to a separate dev branch different then main before we proceed

@Adam-it Adam-it marked this pull request as draft March 9, 2025 20:39
@Adam-it Adam-it mentioned this pull request Mar 9, 2025
3 tasks
@hugoabernier hugoabernier marked this pull request as ready for review March 10, 2025 14:25
@hugoabernier hugoabernier merged commit 007dfc8 into pnp:main Mar 10, 2025
1 check passed
@hugoabernier
Copy link
Collaborator

Thanks @sudeepghatak for your sample! Always appreciated!

We'd love to have your sample featured in one of our future community calls.

If haven't done so yet, and you'd be interested on showing this great sample in a public community call, please fill in following form and we'll get you scheduled - aka.ms/community/request/demo

Thank you for sharing your sample with others - you rock! 👏🥇👩‍💻

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.

4 participants