Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding more stuff to the Home page - Trello #17

Merged
merged 5 commits into from May 28, 2019

Conversation

Projects
None yet
2 participants
@Mordax
Copy link
Contributor

commented May 13, 2019

Link to the Trello request: https://trello.com/c/naQPHMmV

  • Link to Favorites playlist
  • Genre list (collapsible) just linked it
  • Platform list (links to search results for that platform) - This kinda took up a lot of vertical space. Feedback would be great
  • A little Help link/text explaining the search system - People should be encouraged to read the Readme, it never gets read
  • Recently played games - Would require too much rework to implement
  • Link to the Trello board of planned features

Visual screenshot:

visual

@@ -191,6 +233,20 @@ export class HomePage extends React.Component<IHomePageProps, IHomePageState> {
onSelectPlaylist(hof, route);
}

private onFavoriteClick = () => {
const { central, libraryData, onSelectPlaylist } = this.props;
let hof = findFavoritePlaylist(central.playlists.playlists);

This comment has been minimized.

Copy link
@TBubba

TBubba May 13, 2019

Member

"hof" stands for "Hall of Fame". Consider renaming it to something more fitting like "fav" or "favorites".

private getFavoriteBrowseRoute = (): string => {
const defaultLibrary = this.props.libraryData.libraries.find(library => !!library.default);
const defaultRoute = defaultLibrary ? joinLibraryRoute(defaultLibrary.route) : Paths.BROWSE;
let hof = findFavoritePlaylist(this.props.central.playlists.playlists);

This comment has been minimized.

Copy link
@TBubba

TBubba May 13, 2019

Member

"hof" stands for "Hall of Fame". Consider renaming it to something more fitting like "fav" or "favorites".

@TBubba

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Thank you for contributing!

Bugs / Issues:

  • Clicking a platform link does not deselect the current playlist
  • Clicking the Favorites Playlist link does not deselect clear the current search (the Hall of Fame button has the same bug, whoopsie)

Genre List:
I don't know exactly what the person who made making the suggestion had in mind when they asked for a genre list. Try consulting the FP staff about what they would like this feature to be like.

Platform list:
I would like it to fetch the list of platforms at run-time, I could implement this in case you don't want to.

Search System Help:
Good call on dropping it. There already is a link to the readme on the home page, that should be enough.

Layout: (This is more of a ramble than anything else)

  • I think the "Extras box" contains a lot of empty space. Try adding some text and/or icons (look at "Quick Start")
  • Platform list:
    • I think the links are a bit too close to each other (making them harder to read)
    • I think the underlines makes it look "cluttery", maybe they should be replaced with something else (like a tinted background, so it still looks clickable, maybe?)
    • Maybe the platforms should be in alphabetical order?

Maybe it should look something more like this (quick change i made in devtools)?
image

@Mordax Mordax referenced this pull request May 25, 2019

Open

Automatic deselection bugs #20

0 of 2 tasks complete
@Mordax

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

I added the specified bugs as a new issue as to not keep cluttering this PR. Awaiting response regarding Genre list from Nosamu.

Platform list is now grabbed dynamically (although there's an extraneous comma at the end of the list which I'm not sure how to get rid of due to React mapping. )

Added some icons that I thought were relevant, let me know if you prefer something else. For some reason, the list refuses to nicely center-align like the rest of the QuickStartItems (and your demo screenshot). Added a tinted background to the platform links and removed the underlines. Most colours seemed a bit garish compared to the rest of the homepage so I just kept it a lighter grey. If you have any preference, let me know.

Platforms are in alphabetical order.

@TBubba

This comment has been minimized.

You forgot to set the key prop. React requires that you set it to a unique value (no sibling element should have the same key) whenever you create and insert elements like this. Setting it to the index of the current item in the array is usually good enough.

Also, the parameter is named list even though it is a single platform name, maybe platform would be more fitting?

const formatPlatforms = platformList.map((platform, index) =>
      <span key={index}>
@TBubba

This comment has been minimized.

Copy link

commented on src/renderer/components/pages/HomePage.tsx in 6394b54 May 26, 2019

I avoid setting the style of an element wherever it is possible (mainly to avoid css specificity creep). Please declare the styles with CSS in styles.css.

You could add an additional class to to "extras box" and use that to select the platform link elements (that's how I did it for the "upgrades box"). Call it something like home-page__box--extras (if you wonder why the names are formatted like this, I sort of follow the BEM naming convention, although not very closely).

Then you could just write a selector like this in styles.css:

/* HomePage Box - Extras */
.home-page__box--extras span a {
  ...
}

Preferably you would put the "extras box" related CSS between /* HomePage Box - Upgrades */ and /* Random Games */, since I try to keep the CSS ordered after where it appears on the page (I find it easier to find stuff this way).

@TBubba

This comment has been minimized.

About removing the trailing comma:

You can compare the index of the array item with the length of the array to find out if it is the last item in the array.

{ (index < platformList.length - 1) ? ', ' : undefined }

@TBubba

This comment has been minimized.

Copy link

commented on src/renderer/util/platform.ts in 6394b54 May 26, 2019

Because this function only gets one property from each game, you can simplify the code a bit.

This variable's use was to store the different objects that in turn stored the games' property values. But since there only is one left, this "wrapping" object is unnecessary and can be removed (along with the types PlatformProps, GamePropPlatformMap and GamePropPlatform).

You can instead store the inner platform object in its own variable and use it directly.

So you can replace the map variable declaration with this:
const map: { [key: string]: true } = {};

And replace all instances of map.platform in this function with just map.

@TBubba

This comment has been minimized.

Copy link

commented on src/renderer/util/platform.ts in 6394b54 May 26, 2019

Nitpick:
The function is called getPlatform even though it gets multiple platforms.
How about getPlatforms?

@Mordax

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

I think I've covered all the stuff you mentioned.

@TBubba TBubba merged commit bb46883 into FlashpointProject:master May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.