Remove price placeholder during thumbnail loading #1431

Merged

miikkatu merged 2 commits from remove-price-placeholder into master

2018-05-03 05:55:44 +02:00

miikkatu commented

2018-05-02 21:12:53 +02:00

(Migrated from github.com)

This would fix #1427.

Note: the actual fix is just to return null in the if (!costInfo) block.

However, committing that was prevented by linter error PropType is defined but prop is never used for multiple props, even if those props are being used.

As a workaround and for less code, I think that it's enough to check props and call fetchCostInfo only in render function. Linter is happy this way. What do you think?

This would fix #1427. Note: the actual fix is just to return null in the `if (!costInfo)` block. However, committing that was prevented by linter error `PropType is defined but prop is never used` for multiple props, even if those props are being used. As a workaround and for less code, I think that it's enough to check props and call `fetchCostInfo` only in `render` function. Linter is happy this way. What do you think?

neb-b (Migrated from github.com) reviewed 2018-05-02 21:12:53 +02:00

miikkatu commented

2018-05-03 07:12:29 +02:00

(Migrated from github.com)

@seanyesmunt This was not a good solution to the problem. I noticed afterwards that calling fetchCostInfo in rendercauses the component state to change, and this warning to the console:

Warning: Cannot update during an existing state transition (such as within renderor another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved tocomponentWillMount.

I tried moving the call to componentWillMountand componentDidMount, but the price was not fetched properly with either.

So, I'd suggest disabling the linter rule that prevents committing the file with only the change mentioned in the first commit. And making another PR to roll back this.

@seanyesmunt This was not a good solution to the problem. I noticed afterwards that calling `fetchCostInfo` in `render`causes the component state to change, and this warning to the console: `Warning: Cannot update during an existing state transition (such as within `render` or another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to `componentWillMount`.` I tried moving the call to `componentWillMount`and `componentDidMount`, but the price was not fetched properly with either. So, I'd suggest disabling the linter rule that prevents committing the file with only the change mentioned in the first commit. And making another PR to roll back this.

neb-b commented

2018-05-03 07:21:14 +02:00

(Migrated from github.com)

@miikkatu I was wondering about that. Reverted

neb-b commented

2018-05-03 07:22:21 +02:00

(Migrated from github.com)

I'll check it out tomorrow to see if we really need to disable the lint rule, we shouldn't need to. I've seen that error come up but I forget what we did.

👍 1