Skip to content
This repository was archived by the owner on Jul 7, 2023. It is now read-only.

Conversation

@devzhen
Copy link
Contributor

@devzhen devzhen commented Sep 21, 2020

image

@devzhen devzhen self-assigned this Sep 21, 2020
@devzhen devzhen force-pushed the feature/functional-components branch from 7f7e80e to 1458f41 Compare September 23, 2020 13:55
@devzhen devzhen requested a review from plzen September 23, 2020 13:58
@devzhen devzhen force-pushed the feature/functional-components branch from cf83fac to 637586f Compare September 29, 2020 07:52
@devzhen devzhen removed the request for review from plzen September 29, 2020 12:36
@devzhen devzhen force-pushed the feature/functional-components branch from 0ec00c4 to 8f3bc4e Compare September 29, 2020 12:42
@devzhen devzhen added enhancement New feature or request dependencies Pull requests that update a dependency file labels Sep 29, 2020
@devzhen devzhen requested a review from plzen September 29, 2020 15:07
@devzhen devzhen force-pushed the feature/functional-components branch from 8f3bc4e to 8430ff4 Compare September 30, 2020 09:04
Copy link
Contributor

@plzen plzen left a comment

Choose a reason for hiding this comment

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

Could you also cover the scenario when our useContainer hooks reuses another our custom hook. For example, create custom useTheme hook and use it in all places instead getting theme from context. How we should write tests for useContainer in this case?

jest.spyOn(AppState, 'removeEventListener');

describe('useContainer', () => {
reactRedux.useDispatch = () => dispatch;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should move mocking redux to global mocks folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

default: jest.fn(() => ({
modalType: 'MY_MODAL',
modalProps: { customProp: 'customProp' },
modalProps: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add { customProp: 'customProp' } to verify that it's mapped properly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

theme,
handleShouldConfirm,
onModalHide,
setShouldConfirm,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't use last 2 values in component, why do you need to export them? Also, you test internal implementation of this props, but should test only external

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t use these values in the component, but I use them in the test.

I need to return value, for example, from useState for the ability to test it changing.

https://react-hooks-testing-library.com/usage/basic-hooks#updates

exports[`Alert component renders correctly closable 1`] = `
<Styled(View)
color="white"
color="#ededed"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try to get the same values that is already in snapshots? We will see that nothing was changed in migration from containers to hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const useContainer = () => {
const theme = useContext(ThemeContext);

const getStyleProps = (type) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you shouldn't use this as functions, can we modify it to return values and use them in component?

Copy link
Contributor Author

@devzhen devzhen Sep 30, 2020

Choose a reason for hiding this comment

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

Did you mean to return object from custom hook?

return {
    theme,
    styleProps: {
      [ALERT_TYPES.error]: { ... },
      [ALERT_TYPES.success]: { ... },
      [ALERT_TYPES.info]: { ... },
    },
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

@devzhen devzhen closed this Sep 30, 2020
@devzhen devzhen reopened this Sep 30, 2020
@devzhen devzhen force-pushed the feature/functional-components branch 2 times, most recently from da04e7b to a83fd8c Compare September 30, 2020 13:17
@devzhen
Copy link
Contributor Author

devzhen commented Sep 30, 2020

@plzen

Could you also cover the scenario when our useContainer hooks reuses another our custom hook. For example, create custom useTheme hook and use it in all places instead getting theme from context. How we should write tests for useContainer in this case?

I created lib/hooks folder and useTheme custom hook.

import { useContext } from 'react';
import { ThemeContext } from 'styled-components';

const useTheme = () => {
  const theme = useContext(ThemeContext);

  return theme;
};

export default useTheme;

In every hook we can use:

const theme = useTheme();

return { theme };

For testing we can use snapshots

@devzhen devzhen force-pushed the feature/functional-components branch from a83fd8c to ad9079e Compare September 30, 2020 13:48
@devzhen devzhen requested a review from plzen September 30, 2020 13:58
@devzhen devzhen force-pushed the feature/functional-components branch from ad9079e to a515613 Compare October 1, 2020 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants