Automattic / bugomattic

Bugomattic is a tool that guides bug reporters to the right actions within large, complex organizations
GNU General Public License v2.0
6 stars 0 forks source link

Add the duplicate search results list #68

Closed dpasque closed 1 year ago

dpasque commented 1 year ago

What Does This PR Add/Change?

For context, read: pciE2j-1VQ-p2#comment-1716

This adds all the duplicate search results view!

A few notable exceptions that are not imp:

A few other changes include...

Testing Instructions

Issues

Related to #
Closes # 59

john-legg commented 1 year ago

Wow, this looks amazing, @dpasque 🎉 I know this was a lot of work haha. In particular, I really liked the following:

Here's a few things I noticed during testing:

  1. Would it be possible to change the tooltips to have a solid background instead of transparent? There's quite a bit of text underneath them now and although it's not too noticeable, it did draw my attention haha.
  2. I'm sure you'll go through everything again, but do you mind capitalizing the open and closed tooltips?
  3. VoiceOver - I noticed that the issue title is read out along with the phrase "10 items" and then it repeats the title again. I'm not sure what the "10 items" part is 😆 Are you getting that as well?
  4. VoiceOver - This may change this with real data, but the issue description seems to be read in three sections?
  5. VoiceOver - Do we want an aria tag for date opened and last updated so it reads out the full date like the tooltips or is that too verbose?
  6. VoiceOver - The reading of repo is a bit weird. Sometimes it's read before the author, sometimes it's read when focusing on the entire row. The other smaller details (author, dates) are read just fine!
  7. VoiceOver - It might be helpful to read the issue status (open or closed) somewhere.

Anyway, those are the only things I noticed! This PR is super solid and it looks exactly like the designs 😮 The code looks great too, so I'll go ahead and approve it! If you'd like me to do a PR for those smaller changes (tooltip stuff), let me know 😄

dpasque commented 1 year ago

Yay, thanks for the review @john-legg! 😄 Your feedback is thorough and excellent, as always! 🎉

Would it be possible to change the tooltips to have a solid background instead of transparent?

Great idea! This is done 👍

I'm sure you'll go through everything again, but do you mind capitalizing the open and closed tooltips?

🤦 Oops. Nice catch 😆 This is fixed 👍

I noticed that the issue title is read out along with the phrase "10 items" and then it repeats the title again

Hmm, I'm not entirely sure what this might be? So, the links are part of a list, so when you navigate to it, it will read the number of items in the list, which is then usually is something like "10 items" -- could that be it? How are you navigating to the list?

VoiceOver - This may change this with real data, but the issue description seems to be read in three sections

Ahh, this! Sorry, I should have put a disclaimer about this. Long story short: this may seem strange to us, but this is actually okay! Shorter story back longer... Because of how we highlight elements, the text is actually broken up into multiple spans. Screen readers will read each span individually. There are a few hacks you can try to override this, but they're actually not really recommended -- this is normal for VOs and shouldn't throw off our users! https://www.tpgi.com/using-the-text-role/ is a good reference if you're curious!

Do we want an aria tag for date opened and last updated so it reads out the full date like the tooltips or is that too verbose? The reading of repo is a bit weird. Sometimes it's read before the author, sometimes it's read when focusing on the entire row. The other smaller details (author, dates) are read just fine!

I've gone now and standardized this, so there's special screen reader versions of all these meta fields. I think it reads nicer! Let me know what you think! 👍

It might be helpful to read the issue status (open or closed) somewhere.

Great idea! I added this 😺

dpasque commented 1 year ago

@john-legg just as an FYI, I'm going to merge this to help with upcoming PR rebases, I'll leave PR #72 open so we can just add any extra fixes there! 😁

john-legg commented 1 year ago

@john-legg just as an FYI, I'm going to merge this to help with upcoming PR rebases

@dpasque that sounds good! I'll just continue the conversation here so I can answer your questions with context.

it will read the number of items in the list, which is then usually is something like "10 items" -- could that be it? How are you navigating to the list?

This usually happens once I start drilling into the row and hit the first "link." After testing on the the updated trunk, it happens when focusing on the open/closed status now instead of the title. By some miracle, I was able to get a screenshot of what it looks like 😆

Screenshot 2023-04-18 at 1 47 12 PM

The link in this row says "11 items" and "10 items" on other rows. Also, in this example, the faux search only returned 5 results.

Because of how we highlight elements, the text is actually broken up into multiple spans. Screen readers will read each span individually.

Good to know! I learned something new 😄

I've gone now and standardized this, so there's special screen reader versions of all these meta fields. I think it reads nicer! Let me know what you think! 👍

Yeah, it looks/sounds very natural and helpful! 🎉 However, I did notice that it reads 4:30 as "4 hours and 30 minutes" though. That might be standard, but just wanted to mention it in case it shouldn't be that way! The new open/closed description is awesome by the way. Thanks for adding that 👍

dpasque commented 1 year ago

@john-legg Ack, I'm finally getting back around to this PR, sorry! 🙇 Thank you for the extra debugging info! 😄

TL;DR -- both of these are expected and okay!

it will read the number of items in the list, which is then usually is something like "10 items" -- could that be it? How are you navigating to the list? ... This usually happens once I start drilling into the row and hit the first "link." After testing on the the updated trunk, it happens when focusing on the open/closed status now instead of the title.

Ah, I see what this is now! It's telling you that this wrapper link element is made up of 10 (or 11, depending on the content) sub elements that have text nodes and can read by the screen reader. That's expected and ideal actually! It lets people know how much content they can explore within the link 👍

However, I did notice that it reads 4:30 as "4 hours and 30 minutes" though. That might be standard, but just wanted to mention it in case it shouldn't be that way!

I explored this, and the content is always being set correctly, mac VO might just interpret it in different ways sometimes. I think this okay, because we're using standard formats from one of the most common date libraries out there, so this should match behavior on other sites! 👍