Skip to content

Conversation

@nasdan
Copy link
Member

@nasdan nasdan commented Sep 4, 2018

Close #30

@nasdan nasdan requested a review from brauliodiez September 4, 2018 12:33
export interface TrackerProps {
trackedPromiseInProgress
export interface ComponentToWrapProps {
area: string;
Copy link
Member

Choose a reason for hiding this comment

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

Should be here are optional?

Copy link
Member

Choose a reason for hiding this comment

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

Or the other way around? the other hoc shouldn't be optional? not sure

Copy link
Member

Choose a reason for hiding this comment

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

I see HOC is optional, because in common cases you want use areas, is something that you can ignore

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, in HOC is optional, and the HOC is always feeding the area to "component to wrap" (default area if it's undefined)

```

## Sample with areas:

Copy link
Member

Choose a reason for hiding this comment

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

Explain here what areas and usage sample

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,45 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`trackerHoc should render component with trackedPromiseInProgress equals false and area equals "default" when render promiseTrackerHoc 1`] = `
Copy link
Member

Choose a reason for hiding this comment

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

Are useful these snaphsots?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it's the way to know the spec result. Instead of you have to create somethink like we did with karma + enzyme:

...
// Assert
expect(component.html()).toEqual(`
<TestSpinnerComponent
  area="default"
>
...
`)

If there are any changes in component implementation, we will see changes on this snapshot too

src/constants.js Outdated
@@ -0,0 +1 @@
export const defaultArea = 'default';
Copy link
Member

Choose a reason for hiding this comment

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

If this is something internal let's place and ID that could not be duplicated by mistake, somethiing like default-area

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thank you

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@brauliodiez
Copy link
Member

About the samples, got an idea, create two samples on stackblitz

https://stackblitz.com/

They will read from typicode JSON API: https://jsonplaceholder.typicode.com/

On the basic sample let's add a button load: the will load two tables and using react-spinner show a single spinner.

On the areas, let's add two load buttons, each of them will trigger only the spinner related with the table(area).

Le'ts add this to the main README.md

@nasdan
Copy link
Member Author

nasdan commented Sep 10, 2018

It's a great idea, but we will need to publish this version to npm before create samples, isn't it?

@brauliodiez brauliodiez merged commit b37e643 into master Sep 10, 2018
@brauliodiez brauliodiez deleted the feature/30-add-areas-tracking-functionality branch September 10, 2018 19:37
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.

3 participants