add support for custom icon for notification widget #6389
Conversation
|
When I took the latest pull sometime back, I didn't see any merge
conflicts. I used following command on my local branch.
git pull --rebase origin master
What am I missing to check here or in my branch ?
On Tue 4. Oct 2022 at 23:16, Jessica Filben ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks like there are some merge conflicts. Can you update your branch with
the latest version of master and resolve the conflicts?
—
Reply to this email directly, view it on GitHub
<#6389 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHEOHXY356SP2WUD2T6HKR3WBSNBHANCNFSM6AAAAAAQ4IVXKU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Best regards,
Narayan
|
|
First check that your fork is setup so that 'upstream' points to the original grommet repo (instructions on how to do this here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/configuring-a-remote-for-a-fork) Then while on your branch you can do |
|
@jcfilben Fixed merge conflicts. |
|
Can you please add a unit test to cover this scenario? |
Hi @ericsoderberghp I have added some test cases to verify the icon prop scenarios. Kindly review :) |
6040556 to
b13c3d8
Compare
| const theme = { | ||
| notification: { | ||
| unknown: { | ||
| icon: <Home />, | ||
| color: 'blue', | ||
| }, | ||
| }, | ||
| }; | ||
| const Test = () => { | ||
| <Grommet theme={theme}> | ||
| <Notification data-testid="test" title="Test title" message="message" /> | ||
| </Grommet>; | ||
| }; | ||
| const { asFragment } = render(<Test />); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
It looks like the snapshot isn't showing the notification as expected for this test. I think if we change this to the following the test will work
| const theme = { | |
| notification: { | |
| unknown: { | |
| icon: <Home />, | |
| color: 'blue', | |
| }, | |
| }, | |
| }; | |
| const Test = () => { | |
| <Grommet theme={theme}> | |
| <Notification data-testid="test" title="Test title" message="message" /> | |
| </Grommet>; | |
| }; | |
| const { asFragment } = render(<Test />); | |
| expect(asFragment()).toMatchSnapshot(); | |
| const theme = { | |
| notification: { | |
| unknown: { | |
| icon: Home, | |
| color: 'blue', | |
| }, | |
| }, | |
| }; | |
| const { asFragment } = render | |
| <Grommet theme={theme}> | |
| <Notification title="Test title" message="message" /> | |
| </Grommet>; | |
| }; | |
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
Thanks @jcfilben , made the changes and tested on local. New Snapshot is being created. :)
What does this PR do?
This PR adds support for having custom icon in the notification component. For reference to issue, please visit this
Where should the reviewer start?
Reviewer should start looking at changes made in
NotificationPropsand from there in Notification.js fileWhat testing has been done on this PR?
I have tested the changes by passing props to Notification component (part of storybook) which is under
storiesfolderHow should this be manually tested?
You can pass the props manually to see if your custom icon is being reflected on the UI or not.
Do Jest tests follow these best practices?
Yes
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
Issue-6168
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes
Should this PR be mentioned in the release notes?
Not sure
Is this change backwards compatible or is it a breaking change?
The change is backward compatible