feat: show status indicator for collection and items

[cazala]

Closes #1545

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/decentraland/builder/DgVhi6otiCpW2oD7mfy9jh23ntLh
✅ Preview: https://builder-git-feat-show-status-indicator-decentraland1.vercel.app

[coveralls]

Pull Request Test Coverage Report for Build 1288429294

• 111 of 163 (68.1%) changed or added relevant lines in 18 files are covered.
• 2 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.9%) to 9.187%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/modules/common/reducer.ts	0	1	0.0%
src/modules/item/utils.ts	12	13	92.31%
src/components/CollectionStatus/index.ts	0	2	0.0%
src/components/ItemStatus/index.ts	0	2	0.0%
src/modules/common/sagas.ts	0	2	0.0%
src/modules/item/selectors.ts	38	40	95.0%
src/modules/common/store.ts	0	4	0.0%
src/modules/item/export.ts	1	6	16.67%
src/modules/item/sagas.ts	9	15	60.0%
src/components/CollectionStatus/CollectionStatus.container.ts	0	8	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/modules/common/store.ts	1	0%
src/modules/item/export.ts	1	46.48%

Totals
Change from base Build 1283274429:	0.9%
Covered Lines:	975
Relevant Lines:	9096

---

💛 - Coveralls

[fzavalia]

ItemStatus and CollectionStatus share the same css and code. They could be abstracted to a general Status component on which a type: 'item' | 'collection' prop is provided.
The container has a minimal difference in both, but it can be handled by using this prop from ownProps

[fzavalia]

We should check why the formatter is doing this.

[fzavalia]

Not a required change, but remember that lines like this can be written like

collection.isPublished && <CollectionStatus collection={collection} />

To avoid the unnecessary null at the end

[fzavalia]

This is just a personal preference

[fzavalia]

Why the undescore?

[fzavalia]

If you mean to say that collection is an unused variable I recommend you just write an underscore without the name. just underscore means unused, underscore and name means private (in vanilla js)

[cazala]

in typescript the leading underscore does tell the linter to ignore that param for the unused param rule, without it the linter would complain. Just _ does work too, but is more vague, and you can't use it if you need to apply it on a few params but not all.

[fzavalia]

Lets use .mockReturnValueOnce just in case.

[fzavalia]

In this particular test is unnecessary, but if you were to add a new one with another mockReturnValue, the second would not work.

[cazala]

The test fails without it because it calls that mock 3 times

[fzavalia]

Instead of passing an empty mapDispatch you can always pass null or nothing at all.

export default connect(mapState)(ItemStatus)

[cazala]

Yea to be honest I just leave them like that to make it easier to connect later on which is usually what happens with most components over time :P (that's why i create the types too even tho they are empty)

[fzavalia]

The convention for unused variables is just an undescore _, no need to declare the name (undescore and name is the convention for private)

[cazala]

mm afaik typescript allows _ if you don't use any variable but also _param if you need to select them individually, ie:

function (_paramA: any, _paramB: any, paramC: any) {
  // use only paramC
}

I think the _param is clearer and i think it's been used like that throughout the codebase

[fzavalia]

Typo in getMostRelevantStauts

[fzavalia]

You can always one line this with

(obj, entity) => ({...obj, [entity.entityId]: entity })

Just personal preference

[fzavalia]

And even go the extra mile by doing

data: action.payload.entities.reduce((obj, entity) => ({...obj, [entity.entityId]: entity }), state.data)

[cazala]

mm it's not the same tho, the complexity of the code I wrote is linear while the complexity of the one using the spread is exponential, every iteration you are going over all the keys of the object again. You can compare the two ways here, you will notice the expression without the spread finishes in 1 ms while the other one takes 24 seconds (24,000 times slower) oops spread the wrong object, it takes 71 ms.

const stuff = []
for (let i = 0; i < 10000; i++) stuff.push(i)
let time = Date.now()
stuff.reduce((obj, i) => {
  obj[i] = i
  return obj
}, {})
console.log('not using spread', Date.now() - time, 'ms')
time = Date.now()
stuff.reduce((obj, i) => ({...obj, [i]: i }), {})
console.log('using spread', Date.now() - time, 'ms')

// not using spread 1 ms
// using spread 71 ms

For small data sets it might not be noticeable but for anything above 100 it starts halting the UI

[cazala]

Regarding the usage of state.data as the initial value of the reduce, that only works when using the spread, but if I'd do that without using the spread within the reduce, it would be mutating

[cazala]

Oh wait haha it's not 23 seconds (i spread the wrong object 😅 ) it's 71 ms, but still way slower

[cazala]

10k entries might sound like a lot, but we have reducers with 10x that (like the tiles on the map), I learned this the hard way once we added a spread in on reducer or selector that handled that amount of entries and it did froze the UI for a couple seconds every time it got computed lol

[fzavalia]

.mockReturnValueOnce

[cazala]

This one i had to change it back to the mockReturnValue because it's called more than once (three times in this test, once of each item).

[fzavalia]

Looks good!
Comments are mostly suggestions.
@cazala I'll be waiting on your opinion of them.