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

feat(theme): themed-style component #176

Merged
merged 14 commits into from
Nov 14, 2018
Merged

feat(theme): themed-style component #176

merged 14 commits into from
Nov 14, 2018

Conversation

artyorsh
Copy link
Collaborator

@artyorsh artyorsh commented Nov 11, 2018

Short description of what this resolves:

  • organizes theme module structure
  • integrates withThemedStyles HOC which creates themed component and styles for it
  • integrates tests checking for theme change and theme override

@artyorsh artyorsh added the ➡️ Next Upcoming changes label Nov 11, 2018
Copy link
Member

@malashkevich malashkevich left a comment

Choose a reason for hiding this comment

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

Need some work, but mostly looks good

@@ -0,0 +1,4 @@
export { ThemeProvider } from './theme-provider.component';
Copy link
Member

Choose a reason for hiding this comment

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

Let's use camelCase for the whole project.

@@ -0,0 +1,5 @@
// TODO(@type): declare theme types

export type ThemeShape = any;
Copy link
Member

Choose a reason for hiding this comment

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

I would rename it to *Type like ThemeType not *Shape.

Copy link
Member

Choose a reason for hiding this comment

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

Let's name the file types.ts

forwardProps,
} from '../service';

export interface Props<P> extends React.ClassAttributes<P> {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's better to rename it to ThemedProps.

Copy link
Member

Choose a reason for hiding this comment

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

I would also change the name of the parameter to T as in withTheme


constructor(props) {
super(props);
this.renderWrappedComponent = this.renderWrappedComponent.bind(this);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we can't define this methods as lambdas?

);
const themedComponent = component.getByTestId(themeConsumerTestId);
expect(themedComponent.props.themedStyle).not.toBeNull();
});
Copy link
Member

Choose a reason for hiding this comment

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

Make sense to add tests which will check if the component will receive new theme when we change it.

@malashkevich
Copy link
Member

I still sure that we have to rename to camelCase

@malashkevich
Copy link
Member

@artyorsh conflicts

@malashkevich malashkevich merged commit d2e43bf into next Nov 14, 2018
@malashkevich malashkevich deleted the feat/themed-styles branch November 14, 2018 15:07
artyorsh added a commit that referenced this pull request Nov 27, 2018
* chore(playground): themed component example

* refactor(theme): module structure

* feat(theme): themed component props forward

* feat(theme): themed-styles component

* refactor(theme): themed-styles component to separate file

* test(theme): themed-styles component tests

* test(theme): describe themed component and styled themed component tests

* chore(playground): remove themed component example

* chore(lint): update tslint to ignore arrow-functions semicolon

* refactor(theme): fix code style issues

* test(theme): add theme change test

* refactor(theme): fix code style issues

* test(theme): add theme provider overrides parent theme test

* refactor(theme): remove untracked files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➡️ Next Upcoming changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants