ccomeaux / boardgamegeek4android

BoardGameGeek application for Android (unofficial)
GNU General Public License v3.0
228 stars 47 forks source link

Fix expansions location #124

Closed tvedeane closed 4 years ago

tvedeane commented 4 years ago

I think expansions should be in Info tab instead of Descr tab.

Open points (unclear or with need for help):

ccomeaux commented 4 years ago

I think expansions should be in Info tab instead of Descr tab.

They should be on the Credits tab. Whether they stay there or move to a different tab is subjective. BGG has a separate Expansions tab, but I've received feedback there are too many tabs already. I'll see how it looks before merging the PR.

Open points (unclear or with need for help):

  • [ ] is it okay to add com.boardgamegeek.ui.widget.GameDetailRow into fragment_game.xml or it should be extracted to separate file like other included layouts above?

They way you've implemented it is fine.

  • [ ] onListQueryComplete method is now duplicated, should I extract it to some common class?

No, I don't think there's enough code there to worry about it.

  • [ ] colour of the icon for extensions doesn't match other icons on the Info tab (it's gray instead of blue-ish), is there a setting to make the colour align?

The colorize method handles this. You already modified the credits tab to remove it. You just need to add it to the GameFragment.

tvedeane commented 4 years ago

They should be on the Credits tab. Whether they stay there or move to a different tab is subjective. BGG has a separate Expansions tab, but I've received feedback there are too many tabs already. I'll see how it looks before merging the PR.

I agree that new tab shouldn't be added. I don't think Credits is the correct one, because it suggests that one will find there information about who contributed to the game. Expansions are more like general information about a game, similar to complexity or amount of players.

The colorize method handles this. You already modified the credits tab to remove it. You just need to add it to the GameFragment.

Thanks for the hint, I've added the code to handle that.