Skip to content

Conversation

@jkaster
Copy link

@jkaster jkaster commented Oct 27, 2021

Added

  • ProjectView
  • ProjectViewScene
  • Project form
    • Preview of the project
    • Descriptions for project type selector
    • Markdown template for description to aid in complete new project submission

Changed

  • "Contestant" project toggle to "Submit this project for judging?"
  • to Outline versions of icons
  • "View Project" on judging action list

Removed

  • "More information" from project list, editing form, and source code

Screenshots

Updated form

image

Preview project on form

(The John [Object] is from bad data in my spreadsheet. Ignore)

image

View project

image

Judging

View project menu

image

View project from judging list

image

const newProject: unknown = {
title: '',
description: '',
description: `<Put a project overview here. Should be at least one paragraph.>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to update/tweak this template. This is what shows up for a new project in the Description field

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internationalize?

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay. A couple of minor things. Ignore comments about i18n. I can approve if you want.

const newProject: unknown = {
title: '',
description: '',
description: `<Put a project overview here. Should be at least one paragraph.>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internationalize?

if (project) {
// TODO Not sure this is needed here
if (projectId === 'new' && project._id) {
history.push(`${Routes.PROJECTS}/${project._id}`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would treat projectId === 'new' as an error. Not sure why you would want to change the current route if you have an id.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was copy/pasta from ProjectEditorScene and I wanted to talk with you about that bit. Now I don't need to!

}
} else {
if (isProjectLoaded) {
dispatch(actionMessage('Invalid project', 'critical'))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is isProjectLoaded an error here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only gets here if project is undefined/null. If the project is "loaded" then something went wrong ... this is also copy/pasta from the Project editor scene


const getMembers = (team: string[]) => {
try {
return team.join(', ') || 'Nobody!'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i18n of Nobody?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a TODO when we start applying i18n

const tech = techDescriptions(project.technologies, availableTechnologies)
const members = getMembers(project.$members)
const view = `# ${project.title}
by ${members}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an i18n markdown component. Worth supporting here? Maybe not as the judges will mostly be English speakers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting about the markdown. Again, not in scope for the upcoming Hack@Home 2021

@bryans99
Copy link
Collaborator

bryans99 commented Oct 28, 2021

Looked at the sample view above but I am not seeing technologies.

Ignore. Figured it out

@jkaster
Copy link
Author

jkaster commented Oct 28, 2021

It's the Uses line

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more questions

invalidProject()
}
} else {
if (isProjectLoaded) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this? Why is it invalid if project is loaded?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if project is not defined but isProjectLoaded is true, I presume that means the project was invalid. This is based on the existing code from ProjectEditor (which I was not the original author of).

const invalidProject = () =>
dispatch(actionMessage('Invalid project', 'critical'))

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this useEffect is removed?

Copy link
Collaborator

@bryans99 bryans99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jkaster jkaster merged commit c55b221 into main Oct 28, 2021
@jkaster jkaster deleted the jk/project_view branch October 28, 2021 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants