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

Copy to clipboard notification #76

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Joseph-Cashmore
Copy link
Contributor

@Joseph-Cashmore Joseph-Cashmore commented Dec 8, 2020

Makes use of "react-notifications" which would be easily usable elsewhere in other components for success, error, info messages etc.

Required installation of react-notifications and its dependencies.

Main import is in App.js,

Notification component then used within listHeader component

LAPTOP-R2LRJ8HV\josep added 2 commits December 7, 2020 14:50
A notification appears when the user copies the share link to the clipboard
@Joseph-Cashmore Joseph-Cashmore requested a review from a team December 8, 2020 16:00
@github-actions
Copy link

github-actions bot commented Dec 8, 2020

Check out a preview at https://dev.thelist.app/refs/pull/76/merge!

@@ -1,4 +1,6 @@
import React from 'react';
import { NotificationContainer } from 'react-notifications';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There does seem to be a warning in VS Code about a "type" for this import. It doesn't stop things from working, but I couldn't find a means of solving this warning. If it's something we should look into further I can do

Copy link
Member

Choose a reason for hiding this comment

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

There's an open issue here that suggests a workaround: minhtranite/react-notifications#29

@@ -18,6 +19,7 @@ const ListHeader = () => {
});
} else if (navigator.clipboard) {
navigator.clipboard.writeText(url);
NotificationManager.success('Get sharing your list with others!', 'Copied to Clipboard!', 3000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"success" can also be changed to error etc for different notification types

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 we should add a test for this, we'll need to mock out 'react-notifications', allowing us to know when it is called.

jest.mock('react-notifications');

We can then simulate clicking on the button and check that the text is copied to clipboard and that the notification is shown

describe('without the web share API available', () => {
  const writeText = jest.fn(); // create a mock function for writeText
  beforeEach(() => {
    // set the navigator items up for our test
    global.navigator.share = null;
    global.navigator.clipboard = { writeText };

    // render and click on the share icon
    const { getByTitle } = render(<ListHeader />);
    const shareIcon = getByTitle('Share this list');
    fireEvent.click(shareIcon);
  });

  it('copies the item to the clipboard', () => {
    expect(writeText).toHaveBeenCalledWith('https://thelist.app');
  });

  it('triggers a notification', () => {
    expect(NotificationManager.success).toHaveBeenCalledWith('Get sharing your list with others!', 'Copied to Clipboard!', 3000);
  });
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks will get a test added :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where I'm supposed to place "jest.mock('react-notifications');" and in what context. I understand when you make a mock function and assign it to a constant, but how does the test know to utilise this line of code?

Copy link
Member

Choose a reason for hiding this comment

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

There's some great documentation about it here, but the short answer is that it replaces every method in react-notifications with a mocked version that:

  1. doesn't call the actual underlying implementation
  2. allows you to check that the methods were called in the way you expect.

https://jestjs.io/docs/en/mock-functions#mocking-modules

@@ -31,3 +31,8 @@ ul, li {
list-style: none;
padding-inline-start: 0px;
}

.notification-success {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason placing this css code in the component file didn't have any affect on the built in styling

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 great, but we should have a go at creating our own version of these, it'll be quite fun 😄

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.

2 participants