Complete rebuild of theme setup#34
Conversation
matt-bernhardt
force-pushed
the
engx-175-rebuild
branch
6 times, most recently
from
550a74f to
9a44f5a
Compare
** Why are these changes being introduced: * It ends up being easier to rebuild this gem from scratch, rather than trying to make piecemeal changes to its infrastructure. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/engx-175 ** How does this address that need: * This deletes everything in the repository, in preparation for starting fresh. ** Document any side effects to this change: * Some of these files will be restored entirely in a later commit, and others will have changes made to them. For now, though, it is easiest to just delete everything.
** Why are these changes being introduced: * After deleting everything in the previous commit, now we need to start over with a fresh start. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/engx-175 ** How does this address that need: * This commits the output of the first command to initialize a new Rails engine, found at https://guides.rubyonrails.org/engines.html ** Document any side effects to this change: * None
matt-bernhardt
force-pushed
the
engx-175-rebuild
branch
from
9a44f5a to
e6bc244
Compare
** Why are these changes being introduced: * Starting from the boilerplate engine, we need to copy/paste the contents of the proof-of-concept gem (Tesseract) which was developed earlier in this ticket. * We also need to remove an unused application layout template in a namespaced folder path, to prevent future confusion. * The tests for the link helper class need to be added. * The theme assets in vendor/assets need to be added to the asset pipeline. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/engx-175 ** How does this address that need: * This copies the key parts of the Tesseract proof of concept into this gem: * link helper class (including tests) * layout partials * vendor/assets materials (including adding them to asset pipeline) * The changes made in these files from the old theme gem will appear in the overall diff. Most of these changes are the addition of new boilerplate files, but the following specific changes should be noted: * The addition of `extend self` to the link helper class, which helps expose `nav_link_to` to implementing apps. * The addition of the csp_meta_tag directive to the application layout * The change of the body class from `app-thing` to `app-tesseract` * Removes the unused application layout template from its namespaced path. The file which gets used is still in the prior location. ** Document any side effects to this change: * None
** Why are these changes being introduced: * The choice to start this branch by wiping the directory and creating a new gem from `rails new plugin` meant that some meta files which are unrelated have been removed (such as the code of conduct and makefile) * Some minor parts of other files are not optimal (quoting of strings, for example) ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/engx-175 ** How does this address that need: * This restores some meta files from the project root, as well as some values from the gemspec file. * The project readme is updated, with an additional step under the usage section. * Some minor changes to the gemspec file, as well as the contents of lib/ ** Document any side effects to this change: * None
** Why are these changes being introduced: * This repository does not yet have our pull request template, nor does it take advantage of our shared ruby workflows in Github actions. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/engx-175 ** How does this address that need: * This copies the pull request template and workflow yml file from another rails app, to bring this in line with our other projects. * Adds and integrates simplecov so the shared workflow does not error. ** Document any side effects to this change: * The theme gem now has test coverage
matt-bernhardt
force-pushed
the
engx-175-rebuild
branch
from
e6bc244 to
ccc2718
Compare
matt-bernhardt
changed the title
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Sorry for the very delayed review. I still intend to test this a bit locally to understand it more fully, wanted to get an initial review in to let you know I was on it (finally).
| <%= render partial: "layouts/head" %> | ||
| </head> | ||
| <body class="app-thing"> | ||
| <body class="app-tesseract"> |
There was a problem hiding this comment.
I agree app-thing was poorly named (clearly we extracted this gem from thing at some point lol). However, I think maybe app-theme or app-mitlibtheme might be clearer later where the class is coming from.
There was a problem hiding this comment.
I agree with the need to choose this name intentionally; I don't object to either of those values, but one of the difficulties I've had when dealing with this graphic identity is that we don't have a name for it. It is just "our branding information" or, maybe, "the brand we got from Pentagram". That can be particularly challenging when talking about identity schemes from previous years, or comparing between two. I'd love for us to have an actual name to refer to this.
It was in that vein that I chose tesseract for this - it isn't perfect, but the network of lines does bring to mind (at least for me) a tesseract.
(All this said, I understand that this PR isn't the venue to try and settle any questions about what to call our identity package - I'll update this, probably to app-theme, in a future commit)
| @@ -0,0 +1,15 @@ | |||
| <!DOCTYPE html> | |||
There was a problem hiding this comment.
Doesn't having this file in the dummy app prevent the engine from using the engine layout?
There was a problem hiding this comment.
Hmm... that's a good question. I'll do some digging on this.
There was a problem hiding this comment.
I'm trying to wrap this work up this month while I'm still on maintenance, but have not made progress on this question yet. It may come down to my Rails being rusty, but I'm going to have to leave this unanswered for the moment.
| require "test_helper" | ||
|
|
||
| class NavigationTest < ActionDispatch::IntegrationTest | ||
| # test "the truth" do |
There was a problem hiding this comment.
I'm not sure we need to keep this empty test file.
There was a problem hiding this comment.
Oh, yeah - this can be removed. I think I was working on some tests that would fit here, but never settled on any.
I'll take it out when I've got a commit to address the dummy app question above.
- Change body class from app-tesseract to app-theme - Remove empty navigation test file Left unaddressed is some feedback about whether the dummy app needs an application.html.erb file
2 participants
This rebuilds the theme gem in an attempt to get the test suite running and also to make sure that we're using the expected gem / engine setup (according to the current release of Rails).
Among the changes, we are adding a new required step that sites need to follow (adding a command to their application controller). This information is added to the project readme, but I'm also bumping the gem's major version because of this breaking change. The new version is 1.0.0.
There are PRs ready for our four implementing applications that will make this change, which I'll set up once this merges.
The means of achieving this is to, effectively:
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
YES