Conversation
…d tiptap for rich text editing and easy form creation.
RLee64
left a comment
RLee64
left a comment
There was a problem hiding this comment.
Some really visually nice work here, however I still have some issues with how this ticket is being addressed. Please take a look at the comments I've left below. It would be great if you could work on the default user event page before stepping into admin functionality.
I think your work on the event dashboard itself has unfortunately led things down the wrong path. In hindsight, it should be entirely possible for you to complete this task without ever touching the main event page (e.g. just working off a page with url Events/temp with a hardcoded event passed in via json). Any discontinuity in fields used should be fixable in a later PR when we get a better understanding of how the fields should look.
I fully respect that unlike other pages (e.g. home, about, sponsors), we do not have a good reference from the main website for how to style things. This makes our lives a bit more difficult, but it's also an opportunity for us to try different ideas for how something might look. You're correct in that we don't care about styling at this stage, but definitely still make sure that we're considering broader UX ideas with the size and placement of our elements as well as user flow.
Minor thing - All folders should be snake_cased instead of PascalCased, i.e. (src/components/form not src/components/Form). It looks like src/components/ImageBlock was also incorrectly named (which I missed), could you please change both of these?
| @@ -0,0 +1,115 @@ | |||
| import { useEditor, EditorContent } from "@tiptap/react"; | |||
There was a problem hiding this comment.
Editor doesn't seem to be clickable on any area outside of an 'active' line, is there any way to fix this?
| <Route path="" element={<Home />} /> | ||
| <Route path="About" element={<About />} /> | ||
| <Route path="Events" element={<Events />} /> | ||
| <Route path="Events/:eventId" element={<AdminEventEditor />} /> |
There was a problem hiding this comment.
I think we should link to a general event page instead, do the default user side of things, and then introduce admin editing capavilities afterwards.
Admin functionality is something we consider as an extension of default user functionality. Where sensible, it would be good to build admin functionality from the default user view. This is mainly to make the admin a user with elevated permissions rather than someone who accesses the site differently. I think a good result of this is that everything visible to a user is made visible to an admin without needing extra effort (e.g. custom user view, or logging in with a different account).
| import "../style/common.css"; | ||
| import "../style/editor.css"; | ||
|
|
||
| const eventSchema = z.object({ |
There was a problem hiding this comment.
Since this is an admin-centered thing, min and max restrictions honestly aren't really that necessary (and might end up being cumbersome to deal with), the only thing we really care about is making a field required, so that we make sure admins don't accidentally forget to fill out a field.
| const AdminEventEditor = () => { | ||
| const { eventId } = useParams(); | ||
| const navigate = useNavigate(); | ||
| const [isPreview, setIsPreview] = useState(false); |
There was a problem hiding this comment.
A preview mode really isn't necessary for admins to see. There's just not that much benefit to be had aside from maybe like seeing if the image looks nice as a thumbnail.
| .string() | ||
| .min(5, "Title must be at least 5 characters") | ||
| .max(100, "Title is too long"), | ||
| time: z.string().min(1, "Date and time are required"), |
There was a problem hiding this comment.
We'll need to clarify with our clients, but it might be the case where we'll want to leave the time part of datetime ambiguous. And this is just because an event might not have an associated start time (e.g. whole day event), or admins want to clearly outline the start and end times vs just saying the start time.
No need to address for now, but good to keep in mind.
| .regex(/^\d*(\.\d{1,2})?$/, "Enter a valid amount") | ||
| .optional() | ||
| .or(z.literal("")), | ||
| nonMemberPrice: z |
There was a problem hiding this comment.
So the way pricing works for KAC currently is that non-member pricing is the memberPrice + membershipFee, so they become a member and sign-up at the same time. What we'll do instead is a streamlined approach, where users who are not signed in, or are signed in but have not paid to be a member this year, get a different button (e.g. 'sign-up to register') in place of the default registration button, which takes them to the sso auth or stripe respectively.
As a result of this KAC does not have a separate nonMemberPrice
| register, | ||
| placeholder, | ||
| }) => { | ||
| const handlePriceKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
Is this all necessary? Wouldn't it be simpler to just use a number field input and then to just round to 2dp?
There's also a bug where deleting the period or adding a number after the period results in a temporarily invalid value (shows as invalid until another form input). Furthermore, leaving a period does not show an error until the next form input (at which point an error is raised).
| export const DEFAULT_EVENT_IMAGE = "src/images/event-image.png"; | ||
| export const DEFAULT_EVENT_LABEL = "UPCOMING EVENT"; | ||
| export const DEFAULT_USER_ACTION = "SIGN UP"; | ||
| export const DEFAULT_ADMIN_ACTION = "EDIT EVENT"; |
There was a problem hiding this comment.
As an extension of my comment in AdminEventEditor.tsx
I think we should link to a general event page instead, do the default user side of things, and then introduce admin editing capavilities afterwards.
Admin functionality is something we consider as an extension of default user functionality. Where sensible, it would be good to build admin functionality from the default user view. This is mainly to make the admin a user with elevated permissions rather than someone who accesses the site differently. I think a good result of this is that everything visible to a user is made visible to an admin without needing extra effort (e.g. custom user view, or logging in with a different account).
This should link to a general event page instead.
2 participants
No description provided.