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

Add site title partial #118

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Add site title partial #118

merged 1 commit into from
Feb 20, 2024

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Feb 20, 2024

Why these changes are being introduced:

The GeoData visual design includes a heading at the top of each
page, along with some descriptive text.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/GDT-182

How this addresses that need:

This adds the requested element as a partial, with conditional
copy for GDT versus non-GDT applications. It also moves the label text
to the title attribute, as the label is is now visually redundant but
may still be useful to screen reader users.

Side effects of this change:

  • The input label before form submit is now the same as the placeholder
    text. This feels fine since screen readers might not read placeholder
    text, but they do consistently rely on labels.
  • The copy for non-GDT titles did not come from UXWS and is subject
    to change.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • 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 has been confirmed (or is not needed)

Code Reviewer

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

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-182-ne-aqmlbs February 20, 2024 19:20 Inactive
@coveralls
Copy link

coveralls commented Feb 20, 2024

Pull Request Test Coverage Report for Build 7980315778

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.452%

Totals Coverage Status
Change from base Build 7978620685: 0.0%
Covered Lines: 363
Relevant Lines: 365

💛 - Coveralls

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-182-ne-aqmlbs February 20, 2024 19:28 Inactive
@JPrevost JPrevost self-assigned this Feb 20, 2024
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should just remove this now:

https://github.com/MITLibraries/timdex-ui/blob/main/app/views/layouts/_site_nav.html.erb#L3-L6

If we have a site title and an optional platform name in the header, the remaining bit in the "nav" seems redundant. The alternative would be something like only displaying this version if PLATFORM_NAME is set but that feels like a side effect and not something we really need to support.

Requested change: hd-2 class on non-Geo site title in the new partial.

The other bits I'm open for change or not depending on how you prefer.

app/views/basic_search/index.html.erb Show resolved Hide resolved
<h1 class="hd-2">Search for Geographic/GIS data</h1>
<p>Find GIS data held at MIT and other universities</p>
<% else %>
<h1>Search the MIT Libraries</h1>
Copy link
Member

Choose a reason for hiding this comment

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

We likely want to use the same class for non-GDT. hd-2 feels like a better size than our plain h1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good call. That was an oversight on my part.

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-182-ne-aqmlbs February 20, 2024 21:27 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-182-ne-aqmlbs February 20, 2024 21:28 Inactive
@jazairi jazairi requested a review from JPrevost February 20, 2024 21:28
Why these changes are being introduced:

The GeoData visual design includes a heading at the top of each
page, along with some descriptive text.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/GDT-182

How this addresses that need:

This adds the requested element as a partial, with conditional
copy for GDT versus non-GDT applications. It also moves the label text
to the `title` attribute, as the label is is now visually redundant but
may still be useful to screen reader users.

Side effects of this change:

* The input label before form submit is now the same as the placeholder
text. This feels fine since screen readers might not read placeholder
text, but they do consistently rely on labels.
* The copy for non-GDT titles did not come from UXWS and is subject
to change.
* The conditional title-in-nav has been removed from `site_nav`.
This seems superfluous since we now have h1 tags.
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-182-ne-aqmlbs February 20, 2024 21:36 Inactive
@jazairi jazairi merged commit 5c3f4d4 into main Feb 20, 2024
5 checks passed
@jazairi jazairi deleted the gdt-182-new-title branch February 20, 2024 21:59
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.

4 participants