Report Package implementation revision #1585
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,47 @@ | ||
| import "./ReportPackage.css"; | ||
| import { useReducer, useState } from "react"; | ||
|
|
||
| import { | ||
| Modal, | ||
| NewAlert, | ||
| NewButton, | ||
| NewIcon, | ||
| NewSelect, | ||
| NewTextInput, | ||
| useToast, | ||
| type SelectOption, | ||
| } from "@thunderstore/cyberstorm"; | ||
| import { | ||
| type RequestConfig, | ||
| type PackageListingReportRequestData, | ||
| packageListingReport, | ||
| } from "@thunderstore/thunderstore-api"; | ||
| import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
|
|
||
| import { useStrongForm } from "cyberstorm/utils/StrongForm/useStrongForm"; | ||
| import { | ||
| faFaceSaluting, | ||
| faFlagSwallowtail, | ||
| } from "@fortawesome/pro-solid-svg-icons"; | ||
|
|
||
| export function ReportPackageButton( | ||
|
There was a problem hiding this comment. I very much on purpose extracted the components to separate files. Given that you reviewed those PRs I assume you know this. So why bring them back into one file? If the reason is the one mentioned in the commit message, that this is how things are done in the repo, then I'd like to point out that the reason I split the files is that IMO it shouldn't be done that way. (Or to be accurate, that there is a point where splitting components to more files makes sense.) So IDK what to think of this PR reverting the changes. There was a problem hiding this comment. Why create a whole file for one simple component that is meant to be used in the two use cases? I've felt/assfeelt that creating files for each component regardless of their usage and scope, promotes people over-engineering those components. This component happens to be used in two places because of (excuse my french) fucking stupid design decisions, so a need for a common component has arised. (to be used at the two places and as fallbacks) Not sure where the pattern of "create a new file for each component" stems from with React related development, but there absolutely is no need for it. Especially when the LOC is under 300 and the complexity of the said component is extremely low. There was a problem hiding this comment.
We're not going to run out of files, so what is your actual concern here?
I'd rather disregard assfeels; instead I welcome rational, justified arguments. I'd also refrain from pondering what "people" might do and focus on this code base and the files relevant to this PR. Do you think the components I extracted and are now dropped in this PR were overengineered somehow?
What does this have to do with whether the component internally consists of 1 or n files?
It's not React related, it's universal to all programming. Think of a function that grows too long. You split a logical substep into a helper function and name it something like "doSubStep()". If done correctly this abstraction reduces cognitive load and makes following the code easier. Mind you, there's plenty more arguments for splitting things up, but the aforementioned is the most relevant to my argument here. And yes, too much of good thing can be a bad thing, but IMO the split here was net positive.
While there's no fixed limit for the file length, I'd say 300 LOC is well into the orange zone where splitting it up may make sense. And I would argue that anything that currently uses the "strong form" can't be considered "extremely low" complexity. That's why I wanted to have a file/component that contains only that business logic, stripped of all distractions. In this case it meant that the stripped out parts were small by themselves, but combined they're something like 1/3 of the whole. (I've excluded the useReportPackage file from the above calculations as I'm fine with dropping it, as long long as the state management contained in it is not rammed into the form component). (Also from the naming perspective, you now have a ReportPackageForm component that renders a modal which may or may not display a form, which may be a bit confusing.) Generally speaking, the splitting of files is not a yes/no issue. In some places it makes sense, in others it's overkill. But I would urge you to consider if a UI component can be considered "extremely low complexity", if it handles internal state management, UI layout including two separate steps, form validation, API request/response, and error handling? |
||
| props: React.HTMLAttributes<HTMLButtonElement> | ||
| ) { | ||
| return ( | ||
| <NewButton | ||
| tooltipText="Report Package" | ||
| csVariant="secondary" | ||
| csModifiers={["only-icon"]} | ||
| {...props} | ||
| > | ||
| <NewIcon csMode="inline" noWrapper> | ||
| <FontAwesomeIcon icon={faFlagSwallowtail} /> | ||
| </NewIcon> | ||
| </NewButton> | ||
| ); | ||
| } | ||
|
|
||
| ReportPackageButton.displayName = "ReportPackageButton"; | ||
|
|
||
| const reportOptions: SelectOption<PackageListingReportRequestData["reason"]>[] = | ||
| [ | ||
|
|
@@ -27,29 +54,20 @@ const reportOptions: SelectOption<PackageListingReportRequestData["reason"]>[] = | |
| { value: "Other", label: "Other" }, | ||
| ]; | ||
|
|
||
| export interface ReportPackageProps { | ||
| community: string; | ||
| namespace: string; | ||
| package: string; | ||
| config: () => RequestConfig; | ||
| } | ||
|
|
||
| export function ReportPackage(props: ReportPackageProps) { | ||
| const { config, ...requestParams } = props; | ||
|
|
||
| const [submitted, setSubmitted] = useState(false); | ||
| const [modalOpen, setModalOpen] = useState(false); | ||
|
|
||
| const toast = useToast(); | ||
|
There was a problem hiding this comment. As per the commit message from my PR, not showing the error in a toast was also a deliberate choice. Form errors should be shown on the form. Ideally modal related errors should be shown in the modal. Using toasts blinking in the corner of the eye is a bad way to do it. There was a problem hiding this comment. There has been no design decision made on the "way of showing errors" so in efforts of keeping the implementations similar, I've switched it to using the Toast in addition to having Alerts in the form.
The Toasts are meant to convey that something went wrong with submissions or actions, not that the form has something wrong. Hence the utility for grabbing field errors etc in the strongForm. This form just can't have field errors in practice, so the only errors that should show are the submissions related errors. Your thinking is not wrong, but you've confused two different things together There was a problem hiding this comment.
Design decision has now been made. Generally speaking the defaults are:
Note that I'm not asking you to change everything everywhere all at once, just to not actively undo the changes already merged with the changes in this PR. |
||
|
|
||
| function formFieldUpdateAction( | ||
| state: PackageListingReportRequestData, | ||
|
|
@@ -95,21 +113,24 @@ export function ReportPackageForm(props: ReportPackageFormFullProps) { | |
| inputs: formInputs, | ||
| submitor, | ||
| onSubmitSuccess: () => { | ||
| setSubmitted(true); | ||
| }, | ||
| onSubmitError: (error) => { | ||
| let message = `Error occurred: ${error.message || "Unknown error"}`; | ||
| if (error.message === "401: Unauthorized") { | ||
| message = "You must be logged in to report a package."; | ||
| } | ||
| toast.addToast({ | ||
| csVariant: "danger", | ||
| children: message, | ||
| duration: 8000, | ||
| }); | ||
| }, | ||
| }); | ||
|
Comment on lines
118
to
129
There was a problem hiding this comment. Consider removing duplicate error display. Submit errors are shown in two places:
This creates redundant and inconsistent error messaging. Consider either:
Apply this diff to remove the redundant form alert for submit errors: - {strongForm.inputErrors ||
- strongForm.refineError ||
- strongForm.submitError ? (
+ {strongForm.inputErrors || strongForm.refineError ? (
<div className="report-package__block">
{strongForm.inputErrors ? (
<NewAlert csVariant="danger">
@@ -189,10 +189,6 @@
Error while refining: {strongForm.refineError.message}
</NewAlert>
) : null}
- {strongForm.submitError ? (
- <NewAlert csVariant="danger">
- Error while submitting: {strongForm.submitError.message}
- </NewAlert>
- ) : null}
</div>
) : null}
🤖 Prompt for AI AgentsThere was a problem hiding this comment. The toast will not be reintroduced here There was a problem hiding this comment. @anttimaki, understood! I'll keep both error displays as you prefer. |
||
|
|
||
| const form = ( | ||
| <> | ||
| <Modal.Body className="report-package__body"> | ||
| <div className="report-package__block"> | ||
| <label htmlFor="reason" className="report-package__label"> | ||
| Reason | ||
|
|
@@ -147,22 +168,91 @@ export function ReportPackageForm(props: ReportPackageFormFullProps) { | |
| rootClasses="report-package__textarea" | ||
| /> | ||
| </div> | ||
| {strongForm.inputErrors || | ||
| strongForm.refineError || | ||
| strongForm.submitError ? ( | ||
| <div className="report-package__block"> | ||
| {strongForm.inputErrors ? ( | ||
| <NewAlert csVariant="danger"> | ||
|
There was a problem hiding this comment. Input specific errors should be shown next to the relevant input if at all possible. There was a problem hiding this comment. Not possible currently, as that would need mapping of fields to errors and anchoring to elements on the form. Additionally there has not been a design decision made on how errors should be displayed, for anything, so by that the errors should not be shown there. There was a problem hiding this comment.
I see this as a major shortcoming in the strong form implementation. Please add a task into Inbox about adding the support for this.
You've used your own judgement in plenty of places and situations within the project, so saying that something shouldn't be done at all just because no one has explicitly told you to do it is a lazy argument. Keep in mind that the point is to create a good service, not playing company politics or something like that. |
||
| {Object.entries(strongForm.inputErrors).map(([key, value]) => ( | ||
| <div key={key}> | ||
| <strong>{key}:</strong>{" "} | ||
| {Array.isArray(value) ? value.join(", ") : value} | ||
| </div> | ||
| ))} | ||
| </NewAlert> | ||
| ) : null} | ||
| {strongForm.refineError ? ( | ||
| <NewAlert csVariant="danger"> | ||
| Error while refining: {strongForm.refineError.message} | ||
|
There was a problem hiding this comment. IDK what refining means and I assume neither does the users. There was a problem hiding this comment. Refining the form data is done when the input data doesn't match the wanted data format of the endpoint. While this class of errors is rare, there's no reason to not include displaying of these errors, as it'll inform the user that something has gone awry and give more detail to debug the issue. And of course again have to keep in mind; There has been no design decision made on how displaying of errors is to be done regarding the UX, so pretty much anything goes until then. There was a problem hiding this comment.
Thanks for the clarification, but telling this to me doesn't help our less tech-savvy users when they encounter the error message in production. What sort of error messages can we expect from |
||
| </NewAlert> | ||
| ) : null} | ||
| {strongForm.submitError ? ( | ||
| <NewAlert csVariant="danger"> | ||
| Error while submitting: {strongForm.submitError.message} | ||
|
There was a problem hiding this comment. This now contains the less user-friendly error message when attempting to report a package while unauthenticated. There was a problem hiding this comment. This component is the wrong place to re-define the error message, it should be done in the function that throws the error or at very minimum some a bit more shared function like strongForm. At one point I tried to create an generic error parser, but put that development on hold because something else came up. There was a problem hiding this comment.
User doesn't care where the redefinition gets done as long as it gets done. Consequently I don't care where it gets done as long as it gets done, but now it seems it doesn't get done at all, so the change in this PR makes things worse. |
||
| </NewAlert> | ||
| ) : null} | ||
| </div> | ||
| ) : null} | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| <Modal.Close asChild> | ||
| <NewButton csVariant="secondary">Cancel</NewButton> | ||
|
There was a problem hiding this comment. Cancel no longer resets the form state. At least I'd expect it to do so. There was a problem hiding this comment. Hmm, I don't know, I'd expect it to not reset the form state 🤷 This is a UX design decision There was a problem hiding this comment. Currently the poll in our Discord has 100% (n=3) in favor of Cancel resetting the form contents. A LLM also agrees, adding a note that if you're worried about clicking in the modal background accidentally resetting the form for the user, it's behaviour can be different. But that leaves using the X-button or esc-key as a grey area, where closing the modal is unlikely to be accidental. I'd say it's clearer to just reset the form. |
||
| </Modal.Close> | ||
| <NewButton csVariant="accent" onClick={strongForm.submit}> | ||
| Send report | ||
| </NewButton> | ||
| </Modal.Footer> | ||
| </> | ||
| ); | ||
|
|
||
| const done = ( | ||
| <> | ||
| <Modal.Body className="report-package__body report-package__body--centered"> | ||
| <div className="report-package__block "> | ||
| <NewIcon | ||
| csMode="inline" | ||
| noWrapper | ||
| rootClasses="report-package__submitted-icon" | ||
| > | ||
| <FontAwesomeIcon icon={faFaceSaluting} /> | ||
| </NewIcon> | ||
| </div> | ||
| <h3 className="report-package__heading">Thank you for your report</h3> | ||
| <p className="report-package__paragraph"> | ||
| We've received your report and will review the content shortly. | ||
| <br /> | ||
| Your feedback helps keep our community safe. | ||
| </p> | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| <Modal.Close asChild> | ||
| <NewButton csVariant="secondary">Close</NewButton> | ||
| </Modal.Close> | ||
| </Modal.Footer> | ||
| </> | ||
| ); | ||
|
|
||
| return ( | ||
| <Modal | ||
| titleContent="Report Package" | ||
| csSize="small" | ||
| disableBody | ||
| onOpenChange={(isOpen) => { | ||
| if (modalOpen && submitted && !isOpen) { | ||
| // If the modal is being closed after a successful submission, | ||
| // reset the form state. | ||
| updateFormFieldState({ field: "reason", value: "Other" }); | ||
| updateFormFieldState({ field: "description", value: "" }); | ||
| setSubmitted(false); | ||
| } | ||
| setModalOpen(isOpen); | ||
| }} | ||
| trigger={<ReportPackageButton />} | ||
| > | ||
| {submitted ? done : form} | ||
| </Modal> | ||
| ); | ||
| } | ||
|
|
||
| ReportPackage.displayName = "ReportPackage"; | ||
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.
Figma uses
30rem for form part 35rem for "submitted" part of the modal(I could've sworn the widths were different earlier) 39rem, if we're manually setting the width we should probably use that.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'd prefer sticking to pixel widths when there are media width queries being used for the same element. I see no clear benefit of tying the Modal widths to rems as we probably don't want the modal to widen or narrow just when the base font size changes.
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.
There's a sweet spot range for row width, measured in characters, when aiming for good readability. So assuming the modal's contents are in text and e.g. images, it would make sense to me for the modal to widen or narrow when the base font size changes.
However, based on a discussion with the designer, the main takeaway was that we might want to have the modals to default to 100% width (mobile first) and set max width to limit the width on wide displays. The px/rem discussion was less conclusive, so this is not a hill for me to die on, especially as I think there's a decent change we'll be revisiting the issue in the near future.