-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[#69702] Primerize API settings form #21365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1f0e99e to
d59232b
Compare
d59232b to
3eb4e6d
Compare
0fa0e7c to
80155a0
Compare
| <%= render(OpPrimer::InlineMessageComponent.new(scheme: :warning, tag: :p, size: :small)) do %> | ||
| <%= I18n.t(:setting_apiv3_write_readonly_attributes_warning) %> | ||
| <% end %> |
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.
@HDinger I don't like the fact that Inline Message sets its own line-height which causes a mismatch with the rest of the caption text.
This could be overridden ofc:
| <%= render(OpPrimer::InlineMessageComponent.new(scheme: :warning, tag: :p, size: :small)) do %> | |
| <%= I18n.t(:setting_apiv3_write_readonly_attributes_warning) %> | |
| <% end %> | |
| <%= render(OpPrimer::InlineMessageComponent.new(scheme: :warning, size: :small)) do %> | |
| <%= render(Primer::Beta::Text.new(tag: :p, classes: "lh-condensed")) do %> | |
| <%= render(Primer::Beta::Text.new(tag: :strong).with_content("#{I18n.t(:warning)}:")) %> | |
| <%= I18n.t(:setting_apiv3_write_readonly_attributes_warning) %> | |
| <% end %> | |
| <% end %> |
vs
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.
(however, with a condensed line-height, the text no longer baseline aligns with the alert icon)
3eb4e6d to
cbad1f8
Compare
|
|
||
| sf.check_box(name: :apiv3_write_readonly_attributes) | ||
|
|
||
| sf.fieldset_group(title: I18n.t("setting_apiv3_docs"), mt: 4) do |fg| |
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.
The caller still needs to remember to apply the appropriate margin here. This could probably be determined programmatically - either via CSS or Ruby.
Cross-referencing comments relating to the amount of 24px:
https://github.com/opf/openproject/pull/21601/changes#r2674285846
#21520 (review)
| fg.check_box( | ||
| name: :apiv3_cors_enabled, | ||
| data: { | ||
| target_name: "apiv3_cors_enabled", | ||
| disable_when_checked_target: "cause", | ||
| show_when_checked_target: "cause" | ||
| } | ||
| ) do |apiv3_cors_check_box| | ||
| apiv3_cors_check_box.nested_form( | ||
| classes: ["mt-2", { "d-none" => !Setting.apiv3_cors_enabled? }], | ||
| data: { | ||
| target_name: "apiv3_cors_enabled", | ||
| show_when_checked_target: "effect", | ||
| show_when: "checked" | ||
| } | ||
| ) do |builder| | ||
| CORSForm.new(builder) | ||
| end | ||
| end |
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.
This is not DRY. I'd like to create a better abstraction here!
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.
Pull request overview
This PR primerizes the API Settings form by replacing the old form implementation with a new Primer-based form structure. It also replaces the OpPrimer::WarningText component with a more versatile OpPrimer::InlineMessageComponent that supports multiple schemes (warning, critical, success, unavailable) and sizes.
Key changes:
- Introduces
OpPrimer::InlineMessageComponentto replaceOpPrimer::WarningText, providing more flexibility with multiple schemes and sizes - Refactors the API settings form to use the new
fieldset_groupinput structure with dedicated caption templates - Updates translation keys to separate instructional text from warning messages for better component reusability
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/components/op_primer/inline_message_component.rb |
New component supporting multiple message schemes (warning, critical, success, unavailable) and sizes |
app/components/op_primer/inline_message_component.erb |
Template for the new inline message component |
app/components/op_primer/inline_message_component.scss |
Styling for the inline message component with CSS custom properties |
app/components/op_primer/warning_text.html.erb |
Removed old warning text component template |
app/components/_index.sass |
Imports new inline message component stylesheet |
app/forms/admin/settings/api_settings_form.rb |
New Primer-based form implementation with fieldset groups and nested forms |
app/forms/admin/settings/api_settings_form/apiv3_write_readonly_attributes_caption.html.erb |
Caption template with instructions and warning message |
app/forms/admin/settings/api_settings_form/apiv3_max_page_size_caption.html.erb |
Caption template with instructions and warning message |
app/forms/admin/settings/cors_form/apiv3_cors_origins_caption.html.erb |
Caption template for CORS origins field |
app/views/admin/settings/api_settings/show.html.erb |
Simplified view using the new form component |
app/controllers/admin/settings/api_settings_controller.rb |
Added safe navigation operator to handle nil values |
config/locales/en.yml |
Split translation keys to separate instructions from warnings |
modules/webhooks/app/components/webhooks/outgoing/webhooks/row_component.rb |
Updated to use new inline message component |
spec/forms/admin/settings/api_settings_form_spec.rb |
New spec for the API settings form |
spec/components/op_primer/inline_message_component_spec.rb |
Comprehensive spec for the new component |
spec/components/op_primer/warning_text_spec.rb |
Removed old warning text component spec |
| --inline-message-fgColor: var(--fgColor-success); | ||
| } | ||
|
|
||
| &[data-variant='unavailable'] { |
Copilot
AI
Jan 9, 2026
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.
Inconsistent indentation. The selector for unavailable variant has 1 space instead of 2 spaces like the other variant selectors above it. This should be aligned with the other &[data-variant=...] selectors at lines 23, 27, and 31.
| &[data-variant='unavailable'] { | |
| &[data-variant='unavailable'] { |
HDinger
left a comment
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.
The changes itself look good. I am personally not a big fan of the name InlineMessage. To me, this does not transport well enough what this is actually doing... Maybe InlineStatusMessageComponent, InlineStateIndicatorComponent or InlineNoticeComponent? Wdyt?
| @@ -35,21 +35,51 @@ module OpPrimer | |||
| # uses a leading alert Octicon for additional emphasis. This component | |||
| # is designed to be used "inline", e.g. table cells, and in places | |||
| # where a Banner component might be overkill. | |||
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.
At the beginning of this comment, it stills says that this component is used to display warnings
It was previously named |
Okay, so it belongs to the PVC repo after all 👍 |
cbad1f8 to
ab44ec7
Compare
Introduces both an unstyled, low-level `FieldsetComponent` and the `fieldset_group` input group for use with the Primer Forms DSL. https://community.openproject.org/work_packages/70200
Co-authored-by: myabc <[email protected]>
Co-authored-by: myabc <[email protected]>
Aligns implementation with Primer React's InlineMessage component. See: https://primer.style/product/components/inline-message/
ab44ec7 to
21a0a3d
Compare
Ticket
https://community.openproject.org/wp/69702
What are you trying to accomplish?
Primerize the API Settings form. Uses the
fieldset_groupinput group introduced in #21601.Screenshots
What approach did you choose and why?
A best effort.
The "warning text" styling still needs to be discuss.
Merge checklist