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
Show project cards in the public gallery #14869
Conversation
@@ -1,5 +1,6 @@ | |||
import React, {Component, PropTypes} from 'react'; | |||
import ToggleGroup from '../ToggleGroup'; | |||
import msg from '@cdo/locale'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: most places lately we've been using i18n instead of msg
/> | ||
</div> | ||
))} | ||
<h2 style={styles.labHeading}> {i18n.projectTypeGamelab()} </h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot of repeated code across these. Wonder if we should just make a simple ProjectCardList class (maybe even in the same file). Could be as simple as:
const ProjectCardList = ({list}) => ({
list.map((project, index) => (
<div ...>
<ProjectCard .../>
</div>
)
})
let i = 0; | ||
projectTypes.forEach(type => { | ||
publicProjects[type] = []; | ||
for (i=0; i < 5; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (and feel free to ignore) - i tend to find these sorts of things to be easier to read if I do it more functionally, i.e.:
publicProjects.type = _.range(5).map(index => {
// etc
});
``
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I looked for a way to also map the projectTypes
array directly into an object, but I didn't see any syntax that I loved. Or maybe I just couldn't find any that totally avoided using =
or semicolons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few small comments, but generally 👍
}, | ||
{ | ||
name: 'Public Gallery without student info', | ||
description: 'Public gallery sorted by project type AND recency of when the project was added, without student name and age.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the Stories also reflect that the sorting/grouping isn't happening yet? Or is it still applicable since they are sorted/grouped server side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Erin007 I've reworded these, feel free to take another look
Looks like some of the cards are smaller than others? For example, the second card in the PlayLab row. Is that just because the placeholder image is smaller than thumbnails? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wahoo! Happy my start on the cards was useful. Looks awesome, Dave.
Good catch Erin, the playlab card you're talking about appears smaller because it doesn't have a project name. We'll probably want to specify a fixed size on the area below the thumbnail. |
Thanks @Bjvanminnen and @Erin007 for the feedback. I've addressed all of your comments. Feel free to add any more, otherwise I'll go ahead and merge once I have a green circle run. |
public gallery v1 spec
This PR pulls data about published projects from the new dashboard api (added in #14848) and displays it in the
ProjectCardGrid
andProjectCard
(added by @Erin007, thanks Erin!). In the process I made the following changes toProjectCardGrid
andProjectCard
:projectName
toname
updatedAt
withpublishedAt
@Erin007 , I want to call out that most of these changes aren't things that we could have predicted we'd need when you first started working on the project cards. I know it looks like I've made lots of changes to them, but having these components ready was a big help in getting this work done so thanks for your work on those!
To try this in local development, migrate your pegasus db, then publish all of the projects in your local database via the following mysql commands:
Caveats:
updatedAt
) I'm cutting out now, but we'll definitely need later when we implement project cards for the section projects and My Projects views. I think it will be easy enough to resurrect these from the git history later, rather than trying to pull these out now into a different component that isn't organized by project type.visible changes: