Skip to content
This repository has been archived by the owner on May 31, 2021. It is now read-only.

feat(rating): new customItem and containerStyle properties[Rating] #364

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

JeffGuKang
Copy link
Collaborator

@JeffGuKang JeffGuKang commented Nov 14, 2020

Description

User can customize rating component called customItem and containerStyle using their own component instead of default start image.

Related Issues

#363

Tests

I added the following tests:

it('should simulate customItem and containerStyle props', (): void => {
      const rendered = renderer.create(
        component({
          ...defaultProps,
          customItem: {
            onComponent: <View />,
            offComponent: <View />,
          },
          containerStyle: {
            width: 300,
          },
          testID: 'RATING_ID',
        }),
      );

      rendered.update(component({ value: 3 }));
      expect(rendered).toMatchSnapshot();
      expect(rendered).toBeTruthy();

      rendered.update(component({ disabled: true }));
      expect(rendered).toMatchSnapshot();
      expect(rendered).toBeTruthy();
});

Snapshot is updated by npx jest rating -u

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Run yarn test or yarn test -u if you need to update snapshot.
  • Run yarn lint
  • I am willing to follow-up on review comments in a timely manner.

Copy link
Collaborator

@JongtaekChoi JongtaekChoi left a comment

Choose a reason for hiding this comment

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

This change seems really useful !!
I found a tiny issue but I think this pull request is enough to merge.
You may modify this code, or you may create a different issue to solve it. Because the same problem is found in other codes.

}),
);

rendered.update(component({ value: 3 }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This updated component's total property is undefined. So it seems that this method updates the total value to undefined. And the snapshot shows that the wrapper has only 1 child component. (Because new Array(undefined) makes [ undefined, ] and its length is 1)
I think that's not really what you want.
And it seems to some other parts of using the 'update' method have the same problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I will change this as {...defaultProps, value: 3} in another PR.

"flexDirection": "row",
"justifyContent": "space-between",
"opacity": 1,
"width": "NaNpx",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It caused by the same problem.

@JeffGuKang
Copy link
Collaborator Author

There is a lint error not related this PR while testing.

/home/circleci/dooboo/packages/PinchZoom/index.tsx
  227:63  error  Missing semicolon  semi

image

@JongtaekChoi JongtaekChoi merged commit efc05bc into hyochan:master Nov 18, 2020
@JeffGuKang JeffGuKang deleted the feat/363-rating-custom-image branch December 1, 2020 01:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants