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

Fix title tag #87

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Fix title tag #87

merged 2 commits into from
Jul 23, 2024

Conversation

cjrace
Copy link
Contributor

@cjrace cjrace commented Jul 22, 2024

Pull request overview

Flagged in the accessibility audit that the current title isn't appearing.

Pull request checklist

Please check if your PR fulfils the following:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Tests have been run locally and are passing (run_tests_locally())
  • Code is styled according to tidyverse styling (checked locally with tidy_code())

What is the current behaviour?

Currently you get a blank confused title

image

What is the new behaviour?

The title pulls through, there's no extra blank title

image

Anything else

Prompted in part by our conversation this morning @jen-machin

@cjrace cjrace requested a review from jen-machin July 22, 2024 16:18
@cjrace cjrace marked this pull request as ready for review July 22, 2024 16:18
@cjrace
Copy link
Contributor Author

cjrace commented Jul 22, 2024

Copy link
Contributor

@jen-machin jen-machin left a comment

Choose a reason for hiding this comment

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

Theoretically I have no issue with this change, but have you checked it works as expected with NVDA or similar? Is the plan to always have the title in the header and then have the top level page title as a h1, or is the plan to remove those individual page titles? See the attendance dashboard here:

image

The bit in the red circle is the bit I'm referring to. Slightly off track from this PR but if the plan is to keep those page titles then I think we need to make sure they're more descriptive about the actual page that the user is on.

@cjrace
Copy link
Contributor Author

cjrace commented Jul 23, 2024

Theoretically I have no issue with this change, but have you checked it works as expected with NVDA or similar? Is the plan to always have the title in the header and then have the top level page title as a h1, or is the plan to remove those individual page titles? See the attendance dashboard here:

image The bit in the red circle is the bit I'm referring to. Slightly off track from this PR but if the plan is to keep those page titles then I think we need to make sure they're more descriptive about the actual page that the user is on.

I think I agree - in my head, those titles you've circled would be h1s and separate to the overall title of the app. So more like the title of the tab as we have it now, so in the attendance one that particular view I'd make the h1 'User guide' if that makes sense.

The hope is they'd announce whenever the page changes h1, but the title would stay static. So like in EES where the title is EES, but there's different publication titles as h1s.

(with all the plans to update the UI for the template we can update it to how we want to later)

@cjrace
Copy link
Contributor Author

cjrace commented Jul 23, 2024

And I've not tested this myself with any software, just checked that we no longer had a blank title in the head of the html

@cjrace cjrace merged commit 2f345b4 into main Jul 23, 2024
4 checks passed
@cjrace cjrace deleted the fix-title-tag-in-html-header branch July 23, 2024 08:44
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