Talk application page #36
Conversation
- Introduced `pnpm-lock.yaml` to manage package dependencies. - Created `layout.tsx` for the Talk page, including metadata and structure with Header and Footer components. - Updated `page.css` for consistent styling. - Modified `page.tsx` to display a placeholder structure for the Talk page.
- Deleted , , and for both Talk and Events pages to streamline the application structure. - This cleanup removes unused components and styles, improving maintainability.
…ation pages - Deleted the Talk database files and API route to streamline the application. - Updated the Talk submission page with a comprehensive form for proposal submissions. - Enhanced the Talk submission confirmation page with a user-friendly message and next steps.
… smart top spacing
…ser experience - Redesigned the Talk submission page with a modern layout, including a progress bar and enhanced form fields. - Updated the confirmation page to provide clearer next steps and a more engaging visual presentation. - Added animations and improved accessibility features for better user interaction.
- Renamed the old Talk submission URL to for clarity. - Set a new relative path for the Talk submission URL as to streamline navigation.
samholmes
left a comment
samholmes
left a comment
There was a problem hiding this comment.
A bit of feedback so far.
Requesting a rebase. I expect the rebase really only be two or more commits:
- Add pnpm lock file
- Introduce header-height CSS var and page-class
- Implement discord-based request talk form
There was a problem hiding this comment.
Make atomic commit for pnpm lock file.
app/talk/page.tsx
Outdated
| return ( | ||
| <main> | ||
| {/* Current talks: {JSON.stringify(data)} <br /> */} | ||
| <form className="max-w-screen-md m-auto" action="/api/talk-requests" method="POST"> | ||
| <input type="text" placeholder="Speaker Name" name="speaker" /> | ||
| <input type="text" placeholder="Talk Title" name="title" /> | ||
| <textarea placeholder="Talk Description..." name="description"></textarea> | ||
| <input className="btn hover:bg-accent hover:text-base-100" type="reset" value="Reset" /> | ||
| <button className="btn hover:bg-accent hover:text-base-100">Submit</button> | ||
| </form> | ||
| </main> | ||
| <div> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| <h1>Talk</h1> | ||
| </div> |
There was a problem hiding this comment.
This WIP commit should be rebased away to the final work.
| @@ -1,20 +1,22 @@ | |||
| body { | |||
| font-size: 18pt; | |||
| font-size: 18pt; | |||
There was a problem hiding this comment.
Inconsistent formatting should be solved a single commit: "Fix files with broken formatting"
There was a problem hiding this comment.
Should we add a universal standard to the lint rules?
| @@ -0,0 +1,41 @@ | |||
| import { ReactNode } from "react" | |||
There was a problem hiding this comment.
Not sure I'm convinced we need a whole other layout file for the talk page. We should make sure our layout is flexible enough for both the main page and the other pages. Our site only has one layout design, so we should have only one layout file.
| import { PotionBackground } from "./components/PotionBackground" | ||
| import { organizers } from "./info/organizers" | ||
| import { links } from "./siteConfig" | ||
| import { PotionBackground } from "../components/PotionBackground" |
There was a problem hiding this comment.
This commit "Remove layout, page, and styles for Talk and Events sections" would be removed if we only had one layout file.
There was a problem hiding this comment.
Gotcha, I can restructure to have one layout that's more dynamic instead of using two layouts.
|
|
||
| const discordWebhookUrl = | ||
| process.env.DISCORD_WEBHOOK_URL || | ||
| "https://discord.com/api/webhooks/1398562330798981120/xAfw8ZqO4qMV7BKYjWV1KXtct9jDVHTir7L9nRbfsUKMSwhZePLAzl771U6ZIrTCp-b0" |
There was a problem hiding this comment.
This is a leak. You'll need to delete any API keys associated with this. Remove it and just use the process.env.DISCORD_WEBHOOK_URL.
IIRC, we're deploying using GitHub Pages. Is it possible to include secrets for a GitHub Pages site?
There was a problem hiding this comment.
The webhook is intentionally public, it only gives access to send a message into a discord channel, which we want accessible by anyone. It's in a server action but we could move it to just be a client side script no problem.
There was a problem hiding this comment.
It's possible to include secrets on a GH pages site, but if it's a client script it will end up publicly accessible regardless.
| @@ -0,0 +1,71 @@ | |||
| import { NextResponse } from "next/server" | |||
|
|
|||
| export async function POST(req) { | |||
There was a problem hiding this comment.
Can we even have SSR with GitHub Pages as our deployment?
There was a problem hiding this comment.
Good to know GH Pages is the destination, that won't allow server actions. I can move it to just a client script though, shouldn't be an issue.
| </head> | ||
| <body> | ||
| <StyledComponentsRegistry> | ||
| <Header opacity={0} /> |
There was a problem hiding this comment.
Just use margin-top or padding-top for the page content. We know the height of the header by design, so we should know the space needed for margin/padding. We can define a class this. We could use a css variable for the header height so there's one location for future design changes.
There was a problem hiding this comment.
overrides on browser font settings will result in variable height for the header content. We could use js to determine height on load/resize and update margin-top. We could use a "magic-number" for margin-top that is "good enough" in most situations. We could use position sticky, which is probably what we actually want, but it can have some unintended design consequences if we use any different stacking context. I think that the inclusion of an invisible element is the most consistent and effective way to handle the spacing at top, and is clear in the jsx what the intended purpose is.
| Want to give a presentation at our events? Submit your proposal below. Presentations | ||
| are recorded and broadcasted on our channel. | ||
| </p> | ||
| <div className="min-h-screen bg-gradient-to-br from-primary/10 via-base-200 to-secondary/10 py-4 sm:py-8"> |
There was a problem hiding this comment.
This commit should be squashed with the "Add talk submission form" commit
| export const links = { | ||
| talkSubmissionUrl: "https://forms.gle/6gtGEuL7XExHvc6c9", | ||
| oldTalkSubmissionUrl: "https://forms.gle/6gtGEuL7XExHvc6c9", | ||
| talkSubmissionUrl: "/talk", |
There was a problem hiding this comment.
Squash with the Add talk form commit.
I revised the /talk route to display a form for applying to give a talk. This form uses discord webhooks to post the information directly into a private channel in our discord we can easily review.
I had to restructure the app routing a bit so I could use a new layout for the /talk route without disrupting the way the project is currently structured.
I also added an option for opacity to header as an easy way to create "margin" at the top of the page offsetting the navbar height.