Refactor SiteContentForm and use not-allowed cursor for read-only string fields - #2776
Conversation
Summary of ChangesHello @pt2302, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user interface and internal structure of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the field rendering logic in SiteContentForm.tsx to improve code maintainability and alphabetically reorganizes constants in gdrive_sync/constants.py.
- Extracted inline field rendering logic into separate helper functions (
hideForNewVideoResource,showAsLabel,labelValue, andrenderField) - Replaced a large inline
.map()callback with a cleaner.map(renderField)call - Alphabetically sorted constant definitions in the Python constants file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| static/js/components/forms/SiteContentForm.tsx | Refactored field rendering logic by extracting it into reusable helper functions to improve code readability and maintainability |
| gdrive_sync/constants.py | Reorganized constants in alphabetical order for better code organization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const labelValue = (field: ConfigField) => | ||
| field.name === "gdrive_url" | ||
| ? ((values[field.name] as string) ?? "") | ||
| : filenameFromPath(((values[field.name] as string) ?? "") as string) |
There was a problem hiding this comment.
The double type assertion as string is redundant. The outer type assertion is unnecessary since the result of ?? \"\" is already a string.
| : filenameFromPath(((values[field.name] as string) ?? "") as string) | |
| : filenameFromPath((values[field.name] as string) ?? "") |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the SiteContentForm to improve readability by extracting field rendering logic into a dedicated renderField function and several helpers. This is a great improvement for maintainability. The change to use a Label widget for the Google Drive URL is also implemented. I've added a couple of suggestions to further improve code clarity and ensure consistent DOM structure across form fields.
| const labelValue = (field: ConfigField) => | ||
| field.name === "gdrive_url" | ||
| ? ((values[field.name] as string) ?? "") | ||
| : filenameFromPath(((values[field.name] as string) ?? "") as string) |
There was a problem hiding this comment.
This function can be simplified for better readability. The expression for the filenameFromPath case has a redundant as string cast, and the parentheses can be reduced in both cases.
| const labelValue = (field: ConfigField) => | |
| field.name === "gdrive_url" | |
| ? ((values[field.name] as string) ?? "") | |
| : filenameFromPath(((values[field.name] as string) ?? "") as string) | |
| const labelValue = (field: ConfigField) => | |
| field.name === "gdrive_url" | |
| ? (values[field.name] as string) ?? "" | |
| : filenameFromPath((values[field.name] as string) ?? "") |
| return ( | ||
| <div key={field.name}> | ||
| <label htmlFor={field.name}>{field.label}</label> | ||
| <Field | ||
| as={Label} | ||
| name={field.name} | ||
| value={labelValue(field)} | ||
| className="form-control" | ||
| onChange={handleChange} | ||
| /> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
The current implementation for fields shown as labels results in an inconsistent DOM structure compared to other fields. Specifically, the <label> element ends up outside the form-group div, which is rendered by the Label component. This can lead to styling inconsistencies.
To ensure a consistent structure where the form-group div wraps both the label and the input, I suggest rendering the read-only input directly using Formik's Field component instead of using the Label component. This also makes the implementation self-contained and less dependent on the Label component's internal structure.
return (
<div key={field.name} className="form-group">
<label htmlFor={field.name}>{field.label}</label>
<Field
as="input"
type="text"
id={field.name}
name={field.name}
value={labelValue(field)}
className="form-control"
readOnly
style={{ cursor: "not-allowed" }}
/>
</div>
)
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Overall I think moving things into renderField makes the component easier to understand 👍 I left a question/suggestion for possible future simplification around Page url field.
| </div> | ||
| <Label value={`/pages/${content.filename}`} name="page-url" /> | ||
| </div> | ||
| ) : null} |
There was a problem hiding this comment.
I didn't realize we showed this. It makes sense.
Thoughts:
- the content url isn't really specific to just pages. It could be shown for other things.
- It seems very central to WebsiteContent objects. We could make it mostly not special if we (A) add it as a (method) field on WebsiteContentDetailSerializer, (B) add it to the hugo config as a readOnly string field.
- This makes it a lot less special and lets us control the help text via hugo config, and easily add different help text / title for pages, resources, etc.
- it is still slightly special in that it would be a top-level WebsiteContent property, not nested under metadata, similar to "title" and "markdown". See contentInitialValues
If we did ☝️ , wouldn't we be able to remove all special handling (in this file) related to title? There's still some via contentInitialValues, since it's a top-level property.
There was a problem hiding this comment.
Blocking Question: Mentioned this in slack, but in https://github.com/mitodl/hq/issues/9141 you wrote:
The Google Drive URL field introduced in #2742 does not use the Label widget, resulting in a different appearance from other read-only fields such as File.
I'm curious about the part
... resulting in a different appearance from other read-only fields such as File.
It seems to me that:
- "file" widget is the weird exception, all** other readOnly fields use widget: string and readOnly: true in their configs.
- The styling doesn't seem inconsistent to me? At least if I remove the helptext from the hugo-project config, the grdive_url and File styles seem identical
Maybe: gdrive_url and wayback_url are the only two right now? But I think content_url suggested in #2776 (comment) could be another
The refactoring in this PR generally looks good, but I'm not actually sure we should use Label for gdrive_url?
pt2302
changed the title
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
Looks good. Only request is to move the readonly styling to forms.scss with the rest of the form styling.
| style={{ | ||
| ...(extraProps?.style || {}), | ||
| ...(isReadOnly ? { cursor: "not-allowed" } : {}), | ||
| }} |
There was a problem hiding this comment.
This should be done in forms.scss. (Also... currently the extraProps.style inclusion here isn't doing anything. If extraProps.style does exist, the readonly styling will be overwritten on the next {...extraProps} spread. But the best thing to do is to do the readonly styling in forms.scss anyway.)
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9141.
Description (What does it do?)
This PR refactors
SiteContentFormto make the special-case handling for certain fields clearer. It also modifies all read-only string fields to use thenot-allowedcursor.How can this be tested?
Ensure that no functionality is affected; the only visible change is that any read-only fields (such as the
Google Drive URLfield) will now use anot allowedcursor, similar to theFilefield.