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

Updating "No Contacts Found" to use appropriate tag #521

Merged

Conversation

JonnyMc94
Copy link
Contributor

@JonnyMc94 JonnyMc94 commented Nov 15, 2023

This PR:

Resolves #518

Reproduction Steps:

  1. Log in to an account that has no contacts.
  2. Go to the Contacts tab.
  3. Observe the DOM of the content ("No contacts found").

Result - The content is now contained in a <p> tag. Additionally, as suggested in the Possible Solution, the element containing "Contacts", below the Navbar, has now been updated from a <p> to a <h2> tag.

@xscottxbrownx
Copy link
Contributor

so what would be the h1 of this page?

@JonnyMc94
Copy link
Contributor Author

I see that there is a separate issue raised for that problem here - #519

I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

@xscottxbrownx
Copy link
Contributor

I see that there is a separate issue raised for that problem here - #519

I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

No don’t take anything out of this pr. It was an honest question. Do you feel there breadcrumbs could be the h1?

seems many of us are finding issues with the markup, and i was pondering what to do on the other non-home pages for this issue.

@andycwilliams
Copy link
Member

Hi @JonnyMc94, thanks for contributing.

Quick question, in the issue you ask @milofultz to collaborate on this. Did you receive a response saying so? Just want to make sure existing assignees for issues are aware of what's happening and able to respond.

@JonnyMc94
Copy link
Contributor Author

I see that there is a separate issue raised for that problem here - #519
I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

No don’t take anything out of this pr. It was an honest question. Do you feel there breadcrumbs could be the h1?

seems many of us are finding issues with the markup, and i was pondering what to do on the other non-home pages for this issue.

Ok. To be honest, the breadcrumb probably provides the most context and would make the most sense to be the h1! This same fix would then need to be applied to other pages with a similar layout.

@JonnyMc94
Copy link
Contributor Author

Hi @JonnyMc94, thanks for contributing.

Quick question, in the issue you ask @milofultz to collaborate on this. Did you receive a response saying so? Just want to make sure existing assignees for issues are aware of what's happening and able to respond.

I hadn't realised that it was assigned. I was interested in diving into the code base and just spotted this as a nice first contribution. I will hold off until I get a response in future. If @milofultz was working on this I've no issue with the PR being closed, that's my mistake.

@andycwilliams
Copy link
Member

Hi @JonnyMc94, thanks for contributing.
Quick question, in the issue you ask @milofultz to collaborate on this. Did you receive a response saying so? Just want to make sure existing assignees for issues are aware of what's happening and able to respond.

I hadn't realised that it was assigned. I was interested in diving into the code base and just spotted this as a nice first contribution. I will hold off until I get a response in future. If @milofultz was working on this I've no issue with the PR being closed, that's my mistake.

There's an "Assignees" section in each issue where you can see if anyone has claimed the issue yet. If no one is assigned, it's okay for people to assign themselves. However, if it's already assigned to someone, let them know you're also interested and see if they need help.

@milofultz
Copy link
Contributor

Hi @JonnyMc94, thanks for contributing.
Quick question, in the issue you ask @milofultz to collaborate on this. Did you receive a response saying so? Just want to make sure existing assignees for issues are aware of what's happening and able to respond.

I hadn't realised that it was assigned. I was interested in diving into the code base and just spotted this as a nice first contribution. I will hold off until I get a response in future. If @milofultz was working on this I've no issue with the PR being closed, that's my mistake.

All good, have at it!

@milofultz
Copy link
Contributor

I see that there is a separate issue raised for that problem here - #519
I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

No don’t take anything out of this pr. It was an honest question. Do you feel there breadcrumbs could be the h1?
seems many of us are finding issues with the markup, and i was pondering what to do on the other non-home pages for this issue.

Ok. To be honest, the breadcrumb probably provides the most context and would make the most sense to be the h1! This same fix would then need to be applied to other pages with a similar layout.

My thought with "Contacts" being an h2 is that the page people are landing in here should probably be called something and use that as the h1. And then the tabs have their own h2s inside (in this case, "Contacts"). Don't know the name of the larger page per se (shit examples: "Logged In User Page", "User Information", "Dashboard", I dunno).

@JonnyMc94
Copy link
Contributor Author

JonnyMc94 commented Nov 21, 2023

I see that there is a separate issue raised for that problem here - #519
I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

No don’t take anything out of this pr. It was an honest question. Do you feel there breadcrumbs could be the h1?
seems many of us are finding issues with the markup, and i was pondering what to do on the other non-home pages for this issue.

Ok. To be honest, the breadcrumb probably provides the most context and would make the most sense to be the h1! This same fix would then need to be applied to other pages with a similar layout.

My thought with "Contacts" being an h2 is that the page people are landing in here should probably be called something and use that as the h1. And then the tabs have their own h2s inside (in this case, "Contacts"). Don't know the name of the larger page per se (shit examples: "Logged In User Page", "User Information", "Dashboard", I dunno).

Ok, so would I be right in assuming that that creating a contextual title in a h1 tag for these types of pages can be logged as a separate issue?

This issue was created to address the problem right? #519

@xscottxbrownx
Copy link
Contributor

I see that there is a separate issue raised for that problem here - #519
I'm not sure I see a very suitable candidate for a h1 in the markup. It should provide context as to the page content from an accessibility standpoint. I can back the changes to Breadcrumbs.jsx out of this PR until a solid decision on heading structure has been made if necessary?

No don’t take anything out of this pr. It was an honest question. Do you feel there breadcrumbs could be the h1?
seems many of us are finding issues with the markup, and i was pondering what to do on the other non-home pages for this issue.

Ok. To be honest, the breadcrumb probably provides the most context and would make the most sense to be the h1! This same fix would then need to be applied to other pages with a similar layout.

My thought with "Contacts" being an h2 is that the page people are landing in here should probably be called something and use that as the h1. And then the tabs have their own h2s inside (in this case, "Contacts"). Don't know the name of the larger page per se (shit examples: "Logged In User Page", "User Information", "Dashboard", I dunno).

Ok, that's something to discuss then - currently Contacts and Civic Profile are completely different pages/routes.
Screenshot 2023-11-21 at 9 33 41 AM

@leekahung
Copy link
Contributor

Hey @JonnyMc94 @milofultz what's the progress of this PR? Has you two decide how to proceed with the <h_> tags for the different pages?

Have saw the discussions on Issues #518 and #519 and the work in PR #531. Let me know.

@JonnyMc94
Copy link
Contributor Author

@leekahung There hasn't been a final decision as far as I'm aware. This code be merged and then the issue of h1 tag for each page addressed subsequently? From the discussion above I think a decision on what the h1 is going to be for this page and others like it

@leekahung
Copy link
Contributor

@leekahung There hasn't been a final decision as far as I'm aware. This code be merged and then the issue of h1 tag for each page addressed subsequently? From the discussion above I think a decision on what the h1 is going to be for this page and others like it

Got it. If that's the case think I can approve. The updates here looks alright.

Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

Approved.

@leekahung leekahung merged commit 83be71a into codeforpdx:Development Nov 22, 2023
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.

Bug: (Contacts) "No contacts found" using h2 for content instead of organization
5 participants