Skip to content

Conversation

camertron
Copy link
Collaborator

@camertron camertron commented May 25, 2025

What are you trying to accomplish?

Hey everyone 👋 I'm contracting with the OpenProject team for the next few weeks on a couple of new Primer components they need for their app. The OP team mounts all Primer's component previews inside their app's Lookbook, much like GitHub does inside dotcom's Lookbook. Several of PVC's previews however rely on server-side behavior in controllers and views that live in the demo app, eg. demo/app/controllers and demo/app/views. This means that, while some previews render just fine in OP's Lookbook, some error out or simply don't work.

This PR addresses the issue by moving these controllers and views into PVC's Rails engine. This makes them accessible to a host application via Rails' engine mounting mechanism. In other words, a host app can make everything Just Work™ by adding the following to the app's config/routes.rb:

mount Primer::ViewComponents::Engine, at: "/" # or whatever path you want

I have verified everything works as expected inside OpenProject's app. This change should not affect dotcom or any other GitHub property, as the functionality in question is dev-only (or at least, not user-facing) and maintains all existing functionality.

Can't stop, won't stop!

Integration

No changes necessary in production.

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

Copy link

changeset-bot bot commented May 25, 2025

⚠️ No Changeset found

Latest commit: 8f24ef5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@camertron camertron marked this pull request as ready for review May 26, 2025 03:38
@camertron camertron requested a review from a team as a code owner May 26, 2025 03:38
@camertron camertron requested a review from francinelucca May 26, 2025 03:38
@francinelucca francinelucca requested a review from jonrohan May 28, 2025 22:18
@francinelucca francinelucca removed their request for review May 28, 2025 22:19
@jonrohan jonrohan requested a review from Copilot May 28, 2025 22:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Moves server-side preview controllers and views into the Primer::ViewComponents Rails engine and updates form helper paths to point at the new engine routes.

  • Updates ImmediateValidationForm to use engine routes for auto-check endpoints
  • Adds a suite of preview controllers under app/controllers/primer/view_components
  • Introduces an engine-level ApplicationController for all preview controllers

Reviewed Changes

Copilot reviewed 69 out of 69 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
app/forms/immediate_validation_form.rb Updated auto_check_src to use engine route helpers
app/controllers/primer/view_components/application_controller.rb New base controller for engine previews
app/controllers/primer/view_components/toggle_switch_controller.rb Added preview controller for toggle switch
app/controllers/primer/view_components/select_panel_items_controller.rb Added preview controller for select panel items
app/controllers/primer/view_components/nav_list_items_controller.rb Added preview controller for nav list pagination
app/controllers/primer/view_components/multi_controller.rb Added preview controller for multi-input form
app/controllers/primer/view_components/include_fragment_controller.rb Added preview controller for fragment inclusion
app/controllers/primer/view_components/form_handler_controller.rb Added preview controller for form submission demo
app/controllers/primer/view_components/auto_complete_test_controller.rb Added preview controller for autocomplete behavior
app/controllers/primer/view_components/auto_check_controller.rb Added preview controller for auto-check behaviors
app/controllers/primer/view_components/action_menu_controller.rb Added preview controller for action menu demos
Comments suppressed due to low confidence (2)

app/controllers/primer/view_components/select_panel_items_controller.rb:21

  • The page offset calculation is incorrect. For page 1 it yields -2. It should likely be (params[:page].to_i - 1) * items_per_page.
start_index = (params[:page].to_i - items_per_page) * items_per_page

app/controllers/primer/view_components/toggle_switch_controller.rb:1

  • Consider adding a basic request spec to verify the create endpoint returns the correct status and handles the fail parameter.
# frozen_string_literal: true

@jonrohan
Copy link
Member

Thanks @camertron There's a failure still going on, something about a missing method in the forms previews?

@camertron
Copy link
Collaborator Author

@jonrohan whoops, should be fixed now 😄

@camertron
Copy link
Collaborator Author

@jonrohan oops, looks like there was one more URL helper that needed to be fixed. Should be good now.

Feel like adding me to the list of users allowed to kick off workflow runs? 😉

@jonrohan jonrohan added the skip changeset Pull requests that don't change the library output label May 30, 2025
@jonrohan jonrohan added this pull request to the merge queue Jun 2, 2025
Merged via the queue into primer:main with commit 13ccc28 Jun 2, 2025
28 checks passed
@camertron camertron deleted the move_preview_controllers_to_engine branch June 2, 2025 18:03
@camertron camertron mentioned this pull request Jun 6, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset Pull requests that don't change the library output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants