feat(sytemaddon): Implement sections API and dummy feed. Closes #2848. #2859
Conversation
AdamHillier
added
PR / Needs review
and removed
PR / In progress don't review yet
labels
|
I'm taking this since @sarracini has a lot of review ATM |
|
Looks great! Just a few small issues to fix up, but this looks close to ready to go. |
| <h3 className="section-title">{title}</h3> | ||
| {initialized ? (<ul className="section-list"> | ||
| {rows.map(url => url && <img src={url} />)} | ||
| </ul>) : <p>Uninitialized</p>} |
k88hudson
Jul 17, 2017
Member
Perhaps just empty for now, printing the word Uninitialized seems a bit odd
Perhaps just empty for now, printing the word Uninitialized seems a bit odd
|
|
||
| .section-list { | ||
| padding: 0; | ||
| img { |
k88hudson
Jul 17, 2017
Member
I'm assuming this is for your dummy section, I would probably just put it inline in that component to prevent having to remove stuff from here later.
That said, adding custom CSS might be something we'll have to figure out later
I'm assuming this is for your dummy section, I would probably just put it inline in that component to prevent having to remove stuff from here later.
That said, adding custom CSS might be something we'll have to figure out later
| value | ||
| }; | ||
| }); | ||
|
|
k88hudson
Jul 17, 2017
Member
I think the configuration situation in this file is getting too hard to read / needs some refactoring, but I'm OK with filing a separate issue for it.
I think the configuration situation in this file is getting too hard to read / needs some refactoring, but I'm OK with filing a separate issue for it.
Mardak
Jul 30, 2017
Member
Was an issue filed for this? Why was this written as an Array to convert to a Map to convert to an Array to convert to a Map?
Was an issue filed for this? Why was this written as an Array to convert to a Map to convert to an Array to convert to a Map?
| "https://c1.staticflickr.com/1/204/509363724_1f5d8813d0_b.jpg" | ||
| ]; | ||
|
|
||
| this.DummySectionFeed = class DummySectionFeed { |
k88hudson
Jul 17, 2017
Member
Can you add a comment that explains why this exists
Can you add a comment that explains why this exists
| }); | ||
| it("should remove the correct section on SECTION_DEREGISTER", () => { | ||
| const newState = Sections(oldState, {type: at.SECTION_DEREGISTER, data: "foo_bar_2"}); | ||
| assert.equal(newState.length, 4); |
k88hudson
Jul 17, 2017
Member
assert.lengthOf(newsState, 4)
assert.lengthOf(newsState, 4)
k88hudson
Jul 17, 2017
Member
There are a few instances where you can use this
There are a few instances where you can use this
| const newState = Sections(oldState, action); | ||
| assert.equal(newState.length, 6); | ||
| const insertedSection = newState.find(section => section.id === "foo_bar_5"); | ||
| assert.ok(insertedSection && insertedSection.title === action.data.title); |
k88hudson
Jul 17, 2017
Member
assert.propertyVal(insertedSection, "title", action.data.title);
assert.propertyVal(insertedSection, "title", action.data.title);
| const action = {type: at.SECTION_REGISTER, data: {id: "foo_bar_5", title: "Foo Bar 5"}}; | ||
| const newState = Sections(oldState, action); | ||
| const insertedSection = newState.find(section => section.id === "foo_bar_5"); | ||
| assert.ok(Array.isArray(insertedSection.rows) && insertedSection.rows.length === 0); |
k88hudson
Jul 17, 2017
Member
You could just do assert.deepEqual(insertedSection.rows, []),
You could just do assert.deepEqual(insertedSection.rows, []),
k88hudson
Jul 17, 2017
Member
If you want to test both assertions, i'd go with assert.isArray and assert.lengthOf.
If you want to test both assertions, i'd go with assert.isArray and assert.lengthOf.
|
@AdamHillier it might also be good to add a bit of documentation to the v2 docs folder since others will be using this. Feel free to do that as a separate PR. |
|
Happy to do the remaining points as separate PRs |
|
This probably should have been squashed on merge as it ended up adding the individual |
|
Also, without squashing, trying to automatically bisect will lead to failures if it happens to end up in a middle of a set of related changes where later commits fix an earlier commit's breakage. |
No description provided.