Skip to content
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

SRCH-3876: Remove therubyracer. Use cssbuilding-rails gem and less node package instead #1261

Merged
merged 15 commits into from Aug 11, 2023

Conversation

stevenbarragan
Copy link
Contributor

@stevenbarragan stevenbarragan commented Jul 28, 2023

Summary

  • Replace the therubyracer, less, and less-rails-bootstrap gems with the cssbuilding-rails gem and the less package from node directly.
  • Copy bootstrap less files from less-rails-bootstrap into search-gov vendor's folder
  • Developers will need to run the bin/dev script to start the rails server and the process to watch and compile less files together locally.
  • Alternative developers can run just the yarn run watch command separately from the rails server as well.
  • cssbuilding-rails integrates with the rails assets pipeline and runs yarn run build:css automatically during assets pre-compilation in production and staging environments.

Changes in https://otter.staging.search.usa.gov/admin

Checklist

Please ensure you have addressed all concerns below before marking a PR "ready for review" or before requesting a re-review. If you cannot complete an item below, replace the checkbox with the ⚠️ :warning: emoji and explain why the step was not completed.

Functionality Checks

  • You have merged the latest changes from the target branch (usually main) into your branch.

  • Your primary commit message is of the format SRCH-#### <description> matching the associated Jira ticket.

  • PR title is either of the format SRCH-#### <description> matching the associated Jira ticket (i.e. "SRCH-123 implement feature X"), or Release - SRCH-####, SRCH-####, SRCH-#### matching the Jira ticket numbers in the release.

  • Automated checks pass. If Code Climate checks do not pass, explain reason for failures:

Process Checks

  • You have specified at least one "Reviewer".

@stevenbarragan stevenbarragan force-pushed the remove-therubyracer branch 3 times, most recently from 74c0a52 to adc6451 Compare July 28, 2023 20:55
@stevenbarragan stevenbarragan requested review from a team, MyNameIsMissing, krbhavith, lorawoodford and yogesh27 and removed request for a team, MyNameIsMissing and krbhavith July 28, 2023 21:13
Copy link
Contributor

@lorawoodford lorawoodford left a comment

Choose a reason for hiding this comment

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

@stevenbarragan This is looking really great. I left a few minor comments inline.

But I noticed a blocker just now and wanted to report it asap before finishing this review entirely. There seems to be an issue picking up some of the twitter bootstrap icons that I'm seeing both locally and in your otter stack. This is most obvious if you go to a preview search page for any affiliate and look at the (default) blue submit button. In prod it grabs a white magnifying glass icon, but locally and on otter we're not getting that icon. Happy to dig more shortly:
Screenshot 2023-07-31 at 2 05 51 PM

Thanks!

Gemfile Show resolved Hide resolved
README.md Show resolved Hide resolved
app/assets/stylesheets/sites_ie.css Show resolved Hide resolved
@stevenbarragan
Copy link
Contributor Author

@lorawoodford I figure out how to solve the issue with the asset-url. It's working on the otter stack now.

@lorawoodford
Copy link
Contributor

@stevenbarragan I've been going around on circles with this this morning locally because I'm now blocked from just about everything due to having zscaler sprung on me last night, but didn't want to leave this hanging out here. Perhaps someone with a functioning computer like @ddzz or @krbhavith can give this an additional look.

That said, my gut is that while this definitely works I feel like we shouldn't technically need this since it's been addressed upstream in sprockets-rails: rails/sprockets-rails#476. A small part of me feels we're kicking some maintenance pain down the road a bit here, but I'm inclined to ignore that for now because this set of changes alone is a huge leap forward. Ideally I'd try out a few things here locally myself, but seeing as I can't even yarn install without it getting blocked by zscaler, that's probably not going to happen in a timely manner.

Happy to add an approval once the merge conflict is addressed and once some other person with a working machine give it a quick check. 👍

README.md Show resolved Hide resolved
bin/css Outdated Show resolved Hide resolved
@stevenbarragan
Copy link
Contributor Author

@lorawoodford another AssetUrlProcessor is needed because the one from lib/sprockets/rails/asset_url_processor.rb only works with the url parameter instead of the asset-url like we need it to.

@lorawoodford lorawoodford self-requested a review August 4, 2023 19:11
Copy link
Contributor

@lorawoodford lorawoodford left a comment

Choose a reason for hiding this comment

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

@stevenbarragan Leaving an approval for this, but we should carve out time to discuss test plans next week at the end of daily scrum as this has (as has already been discussed) developer process impacts as well as (the potential for, though there shouldn't be any!) public-facing impact and, as such, requires fed Acceptance Testing by policy.

@lorawoodford
Copy link
Contributor

@stevenbarragan be sure to update the JIRA number in the PR title here to reflect the current story/not the investigation story... that's how the PR gets automatically linked in JIRA.

@stevenbarragan stevenbarragan merged commit 9ca89c0 into GSA:main Aug 11, 2023
12 checks passed
@stevenbarragan stevenbarragan deleted the remove-therubyracer branch August 11, 2023 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants