Skip to content

Conversation

JPrevost
Copy link
Member

Relevant ticket(s):

How does this address that need:

  • Updates markup to match requested changes in linked ticket

Document any side effects to this change:

The 404 template and the default search results template both share the same search form partial. We don't generally expect the default search template to be used, but the changes to 404 do cause that to update as well.

Developer

Secrets

  • All new secrets have been added to Pantheon tiers
  • Relevant secrets have been updated in Github Actions
  • All new secrets documented in README

Documentation

  • Project documentation has been updated
  • No documentation changes are needed

Accessibility

  • ANDI or Wave has been run in accordance to
    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)

Stakeholder approval

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies

NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

@JPrevost JPrevost requested a review from matt-bernhardt April 12, 2023 17:46
@JPrevost
Copy link
Member Author

JPrevost commented Apr 12, 2023

I'll fix those two code climate whitespace issues.

[done]

@matt-bernhardt matt-bernhardt self-assigned this Apr 12, 2023
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

LGTM - I've not read the Jira ticket closely to confirm that the content here is correct, but have faith that the folks in UX have their eyes on that.

Good call on adding a sample URL to the search template, should future-us want to confirm where that gets called (or doesn't).

:shipit:

<p>You can also check out these commonly-used resources:</p>
<p>Need help? <a href="/ask">Ask us!</a></p>

Or try:
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, interesting - I was going to ask that this text be enclosed in a tag, but the W3C validator doesn't flag it as a problem. So nevermind :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's probably part of a div at some point so it's technically fine. However, I'm glad you pointed it out as now it's bugging me and I suspect I'll throw it in a

if it doesn't muck up the display too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've wrapped it in a

tag and it looks fine to me. I'm holding off on merging this until UX signs off. Thanks!

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/LM-270

How does this address that need:

* Updates markup to match requested changes in linked ticket

Document any side effects to this change:

The 404 template and the default search results template both share the
same search form partial. We don't generally expect the default search
template to be used, but the changes to 404 do cause that to update as
well.
@JPrevost JPrevost merged commit 3f599b8 into master Apr 13, 2023
@JPrevost JPrevost deleted the lm270-404s branch April 13, 2023 20:43
matt-bernhardt pushed a commit that referenced this pull request May 10, 2023
carves out an exception to the index.php file in post install
Our post-install command was updated to create hard symlinks to the contents of the web/wp directory. We do not want this for the index.php file which is modified to point to /wp/
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.

2 participants