[#69068] Add support for Project template icons#21035
Conversation
27b9687 to
aa115c0
Compare
aa115c0 to
3f1a0ad
Compare
4050545 to
6f69140
Compare
3f1a0ad to
9b19582
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for customizable template icons for OpenProject projects. It introduces a new Icon model hierarchy to store either Octicon-based or custom uploaded icons, and integrates a custom file upload component using GitHub's Catalyst library.
Key changes:
- New database schema for storing project icons with STI (OcticonIcon, CustomIcon)
- File upload form field using Catalyst-based custom element
- Integration of icons into project settings and template selection
Reviewed Changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds @github/catalyst dependency for custom element decorators |
| package-lock.json | Updates lock file with Catalyst dependency and removes peer: true from glob |
| lib/primer/open_project/forms/file_field.rb | New FileField component for Primer forms |
| lib/primer/open_project/forms/file_field.html.erb | ERB template for file field with custom element integration |
| lib/primer/open_project/forms/dsl/input_methods.rb | Adds file_field DSL method |
| lib/primer/open_project/forms/dsl/file_field_input.rb | Input class for file field with Catalyst target integration |
| frontend/src/main.ts | Imports custom elements module |
| frontend/src/global_styles/openproject/_forms.sass | Styles for file field component |
| frontend/src/custom-elements/primer-file-field.ts | Custom element implementation using Catalyst |
| frontend/src/custom-elements/index.ts | Module entry point for custom elements |
| db/migrate/20251111183800_create_icons.rb | Migration creating icons table with polymorphic association |
| config/locales/en.yml | Translation keys for icon-related UI |
| app/models/project.rb | Includes HasIcon concern |
| app/models/permitted_params.rb | Permits icon parameter |
| app/models/octicon_icon.rb | Model for Octicon-based icons |
| app/models/icon.rb | Base icon model (missing copyright header) |
| app/models/custom_icon.rb | Model for custom uploaded icons |
| app/models/concerns/has_icon.rb | Concern for iconifiable models |
| app/forms/projects/template_select_form.rb | Uses template.icon instead of hardcoded image |
| app/forms/projects/settings/icon_form.rb | Form for uploading/selecting project icons with placeholder values |
| app/contracts/projects/base_contract.rb | Validates icon attribute (incorrect validation logic) |
| app/components/projects/settings/general/show_component.html.erb | Adds icon form section to project settings |
| app/assets/images/templates/pmflexone.svg | New template icon SVG |
| class Icon < ApplicationRecord | ||
| end |
There was a problem hiding this comment.
The Icon model is missing a copyright header. All other ApplicationRecord models in the codebase include the standard OpenProject copyright notice (see color.rb, project.rb, etc. for examples).
| attribute :icon do | ||
| validate_templated_present | ||
| end |
There was a problem hiding this comment.
The validation method validate_templated_present is incorrect for the icon attribute. This validator checks if model.templated.nil? and adds an error to :templated, but it's being used for the :icon attribute. This will validate the wrong attribute and produce confusing error messages. The icon validation should either validate icon-specific requirements or be removed if no validation is needed.
| attribute :icon do | |
| validate_templated_present | |
| end | |
| attribute :icon |
| form do |f| | ||
| f.file_field name: :icon, label: attribute_name(:icon), accept: "foo", size: "15" |
There was a problem hiding this comment.
The accept parameter has a placeholder value 'foo' which should be replaced with appropriate file type restrictions (e.g., 'image/svg+xml,image/png,image/jpeg'). The size parameter value '15' also appears arbitrary and should be documented or replaced with a meaningful constant.
| form do |f| | |
| f.file_field name: :icon, label: attribute_name(:icon), accept: "foo", size: "15" | |
| # The size of the file input field for project icon uploads. | |
| ICON_FILE_FIELD_SIZE = 15 | |
| # Only allow SVG, PNG, and JPEG image uploads for project icons. | |
| ICON_FILE_ACCEPT_TYPES = "image/svg+xml,image/png,image/jpeg" | |
| form do |f| | |
| f.file_field name: :icon, label: attribute_name(:icon), accept: ICON_FILE_ACCEPT_TYPES, size: ICON_FILE_FIELD_SIZE |
| // if (!window.customElements.get('primer-file-field')) { | ||
| // window.PrimerFileFieldElement = PrimerFileFieldElement | ||
| // window.customElements.define('primer-file-field', PrimerFileFieldElement) | ||
| // } |
There was a problem hiding this comment.
Commented-out code should be removed. The custom element registration is essential for the component to work; if this code is needed, it should be uncommented and properly integrated. If it's handled elsewhere, the commented code should be deleted.
| // if (!window.customElements.get('primer-file-field')) { | |
| // window.PrimerFileFieldElement = PrimerFileFieldElement | |
| // window.customElements.define('primer-file-field', PrimerFileFieldElement) | |
| // } | |
| if (!window.customElements.get('primer-file-field')) { | |
| window.PrimerFileFieldElement = PrimerFileFieldElement | |
| window.customElements.define('primer-file-field', PrimerFileFieldElement) | |
| } |
| this.inputElement.addEventListener('change', () => { | ||
| const files = this.inputElement.files!; | ||
| if (files.length === 0) { | ||
| this.label.textContent = 'no file selected'; |
There was a problem hiding this comment.
The hardcoded text 'no file selected' should be internationalized using the i18n system like other user-facing strings in OpenProject. Consider using a translation key or accepting the text as a data attribute from the server-rendered HTML.
| this.label.textContent = 'no file selected'; | |
| this.label.textContent = this.getAttribute('data-no-file-selected-label') || 'no file selected'; |
Ticket
https://community.openproject.org/wp/69068
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Merge checklist