Update docs #723
Update docs #723
Conversation
Adds additional config settings even though most are left on default
Results were sliding behind content. For some reason, this solves the problem.
We initially didn't switch this app to our shared CI config because it was transitioning from ElasticSearch to OpenSearch which required running two separate sets of tests. That work has been completed so we can move to our shared CI as planned.
There was a problem hiding this comment.
I've asked some questions in comments throughout this PR, but none what I ask is a blocker for this merging - just me trying to make sure I understand the change.
My only request is for a follow-up ticket (you can assign it to me if you'd like) to look at the color contrast for the search results in the docs resource.
My general response: 
|
|
||
| jobs: | ||
| shared: | ||
| uses: mitlibraries/.github/.github/workflows/ruby-shared-ci.yml@main |
There was a problem hiding this comment.
It looks like this shared workflow is essentially the same as the ci.yml file that's being removed in this PR? I'm definitely okay with that.
There was a problem hiding this comment.
It's very similar but requires a normalized *.lcov filename. The default is the repo name but apps using the shared ci workflow all have to use the same filename so there is a related change in this repo to allow for moving to the share workflow in test_helper.rb as you noted in a comment on that file. So yes, those two changes together make the shared workflow, er, work :)
| @@ -10,72 +10,72 @@ GIT | |||
| GEM | |||
There was a problem hiding this comment.
As you mentioned on Slack, I'm ignoring the changes to the Gemfile.lock as those are regular dep updates.
| bin/rails test | ||
| ``` | ||
|
|
||
| The following additional manual testing should be performed in the PR build on Heroku. |
There was a problem hiding this comment.
This structure looks pretty clear to me, and I agree with the recommendations being made here. Thanks for including specific steps to take, and including the same searches to perform - that's very repeatable :-)
| # Set the search token separator | ||
| # Default: /[\s\-/]+/ | ||
| # Example: enable support for hyphenated search words | ||
| tokenizer_separator: /[\s/]+/ |
There was a problem hiding this comment.
This looks like the only value we're setting in this section that isn't using the default value? Is that correct?
I know we've chosen various paths around this in the past (whether to specify default values explicitly or let them be assigned a value by the default setup) - I don't have a problem with any particular value being set, nor the choice to set them specifically. I just want to put this change in context.
There was a problem hiding this comment.
Yeah in this case I felt it was useful to understand what the configuration options were even if we were leaving defaults. I'm also not entirely sure why this particular setting isn't using the default. I suspect the default changed between our initial setup and now upstream.
| @@ -63,7 +63,6 @@ body { | |||
|
|
|||
| .main { | |||
| min-height: 100vh; | |||
| display: flex; | |||
There was a problem hiding this comment.
🙈 that this works (I've confirmed that), but I'm mystified why it does.
At some point I think we should look at the background color applied to that area, as the blue-on-gray I don't think passes a contrast check:
(WebAIM has this at a 1.69:1 ratio right now, assuming this change does what I think it will. I'm fine with doing this in a subsequent PR, though - the contrast problem exists now, so we aren't introducing it here.)
There was a problem hiding this comment.
Ah interesting. This is a bug between the light and dark modes. Dark mode the contrast is a bit better. I'll do a quick fix for light mode now as I'm worried it may take us a while to actually get back to this.
There was a problem hiding this comment.
I've increased contrast for this search preview in both light and dark modes. It likely isn't perfect yet but it is better. I am having a bit of a hard time actually getting the contrast report to display for the hidden content so I could use a pairing session at some point to better understand how to do that.
| @@ -3,6 +3,7 @@ | |||
| require 'simplecov' | |||
| require 'simplecov-lcov' | |||
| SimpleCov::Formatter::LcovFormatter.config.report_with_single_file = true | |||
| SimpleCov::Formatter::LcovFormatter.config.lcov_file_name = 'coverage.lcov' | |||
There was a problem hiding this comment.
My assumption is that this filename matches the one being defined in the shared workflow?
Increases contrast for both light and dark modes for the search preview
|
@matt-bernhardt I'm merging but would like to chat more about how to test the contrast for the search preview. I think this is an improvement so I don't want to hold it up, but I wasn't able to confirm if it meets our desired target. We likely need some targeted effort to clean up this theme overall and maybe we can use this contrast issue to spark that discussion. |
Requires database migrations?
NO
Includes new or updated dependencies?
YES