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

Few more tests with a little better desrciption #906

Merged
merged 9 commits into from
Jun 18, 2021

Conversation

mgwojciech
Copy link

Q A
New sample? [X ]

Added unit tests for FileBrowser component. I added comments to walk the developer through my thought process and explain why certain things are implemented this way. Hope You'll find that sample useful.
If there is any component You would like me to do next, let me know.

Have a great one!

@joelfmrodrigues
Copy link
Collaborator

@mgwojciech many thanks for this!
Can you please revert the commit of the package-lock.json refresh and get latest changes from dev branch? The npm install issue was already resolved separately

@mgwojciech
Copy link
Author

@joelfmrodrigues - fixed.

@@ -29,14 +29,13 @@ interface IPreviewImageCollection {

export class ListItemAttachments extends React.Component<IListItemAttachmentsProps, IListItemAttachmentsState> {
private _spservice: SPservice;
private previewImages: IPreviewImageCollection;
private previewImages: IPreviewImageCollection = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mgwojciech looks like all the changes on this file were already previously merged. Could it be from an earlier PR and you forgot to sync your branch since then?
Not an issue as I have checked the code, but something to keep in mind for future PRs to make reviews a little easier.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I thought I pulled everything before pr, I'll pay more attention to this next time

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem at all, was only some files and was simple to compare.

@joelfmrodrigues
Copy link
Collaborator

joelfmrodrigues commented Jun 18, 2021

@mgwojciech just reviewed everything, many thanks! I really need to get more experience in testing 😅
Just one last thing: can you please resolve the merge conflict on package.json?
If you prefer I can resolve it, but the code changes would come as if made by me which I would like to avoid

@joelfmrodrigues
Copy link
Collaborator

@mgwojciech just reviewed everything, many thanks! I really need to get more experience in testing 😅
Just one last thing: can you please resolve the merge conflict on package.json?
If you prefer I can resolve it, but the code changes would come as if made by me which I would like to avoid

@mgwojciech sorry, ignore my previous comment, I found a simple way to do this 🤦‍♂️
Many thanks for the contribution and sorry it took so long to review

@joelfmrodrigues joelfmrodrigues merged commit 152d0e7 into pnp:dev Jun 18, 2021
@estruyf estruyf mentioned this pull request Jun 23, 2021
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.

None yet

2 participants