-
Notifications
You must be signed in to change notification settings - Fork 192
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
feat: markdown, link click support, and project view for hackathon projects #877
Conversation
- removed "more info" from UI because it's not needed any more - Changed icons and some design with Jared
@@ -50,7 +50,14 @@ import { getCurrentProjectsState, getIsProjectMemberState } from './selectors' | |||
const createNewProject = (): IProjectProps => { | |||
const newProject: unknown = { | |||
title: '', | |||
description: '', | |||
description: `<Put a project overview here. Should be at least one paragraph.> |
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.
Feel free to update/tweak this template. This is what shows up for a new project in the Description field
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.
Internationalize?
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 think this is okay. A couple of minor things. Ignore comments about i18n. I can approve if you want.
@@ -50,7 +50,14 @@ import { getCurrentProjectsState, getIsProjectMemberState } from './selectors' | |||
const createNewProject = (): IProjectProps => { | |||
const newProject: unknown = { | |||
title: '', | |||
description: '', | |||
description: `<Put a project overview here. Should be at least one paragraph.> |
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.
Internationalize?
packages/hackathon/src/scenes/ProjectEditorScene/components/ProjectForm.tsx
Outdated
Show resolved
Hide resolved
if (project) { | ||
// TODO Not sure this is needed here | ||
if (projectId === 'new' && project._id) { | ||
history.push(`${Routes.PROJECTS}/${project._id}`) |
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 would treat projectId === 'new' as an error. Not sure why you would want to change the current route if you have an id.
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.
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')) |
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.
Why is isProjectLoaded an error here?
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.
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
packages/hackathon/src/scenes/ProjectViewScene/ProjectViewScene.tsx
Outdated
Show resolved
Hide resolved
|
||
const getMembers = (team: string[]) => { | ||
try { | ||
return team.join(', ') || 'Nobody!' |
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.
i18n of Nobody?
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.
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} |
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.
We have an i18n markdown component. Worth supporting here? Maybe not as the judges will mostly be English speakers.
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.
Interesting about the markdown. Again, not in scope for the upcoming Hack@Home 2021
packages/hackathon/src/scenes/ProjectViewScene/components/index.ts
Outdated
Show resolved
Hide resolved
Ignore. Figured it out |
It's the Uses line |
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.
Couple more questions
invalidProject() | ||
} | ||
} else { | ||
if (isProjectLoaded) { |
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.
Not sure about this? Why is it invalid if project is loaded?
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.
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(() => { |
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.
What happens if this useEffect is removed?
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.
LGTM
Added
ProjectView
ProjectViewScene
Changed
Removed
Screenshots
Updated form
Preview project on form
(The
John [Object]
is from bad data in my spreadsheet. Ignore)View project
Judging
View project menu
View project from judging list