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 semantic HTML to overall analytics and panel wrappers #2750

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

heyainsleymae
Copy link
Contributor

@heyainsleymae heyainsleymae commented Oct 29, 2024

Some initial progress on #2742 that can be merged separately from the more intensive work like menus and tables

Most relevant changes

  • Consolidated header markup and use appropriate heading levels
  • Placed overall analytics boxes in a <ul> and make each statistic an atomic live region
  • Overall analytics are in an <aside>, all panels are inside of the <main>
  • Each data panel is now an <article> with an appropriate programmatic name
  • Menu icons are now buttons
    • They are in need of accessible names, and I did not attempt to add any new labels since I wasn't sure how to handle the line number references in the translations
  • Font Awesome icons have aria-hidden to remove them from the accessible name of their parent (I didn't hide them on the menu buttons so that I remembered to mention the addition of new label IDs)

Other changes

  • There is now one set of menu markup that works for all viewports
  • The main page heading and last updated time remain fully visible in all viewports, stacking when required
  • The overall analytics boxes now also collapse to two columns before fully stacking
  • The WebSocket indicator is now a crimson fa-square when not connected, changing to the same green fa-circle once a connection is established

@heyainsleymae
Copy link
Contributor Author

Barring any other requested changes, the only additional work that this PR needs before being ready to merge is the addition of labels for the menu button, settings button, and WebSocket statuses.

@allinurl allinurl marked this pull request as ready for review November 4, 2024 23:55
@allinurl
Copy link
Owner

allinurl commented Nov 5, 2024

This is looking great! I finally had a chance to take a look, and I can see the structure changes. Overall, it's shaping up nicely, but I did notice a few small issues:

For example, there are some padding and margin changes, particularly with the hamburger menu and the gear icon, as well as with the "Overall Analyzed Requests" section. Also, it would be great if we could maintain consistent styling for the "Last Updated" label to keep everything looking cohesive.

I also noticed that some of the colors in the date range appear slightly different across the various themes, so that might need a little tweaking as well.

Overall, I really appreciate the progress — it's definitely getting there! Feel free to move it out of draft when you think it’s ready, and I’ll be happy to take another look.

@heyainsleymae
Copy link
Contributor Author

heyainsleymae commented Nov 20, 2024

For example, there are some padding and margin changes, particularly with the hamburger menu and the gear icon, as well as with the "Overall Analyzed Requests" section.

The margin and padding of the "Overall" section has been adjusted, but I've got a clarifying question for the menu icons: Would simply moving them lower, closer to their original position, be sufficient, or are you looking for something more than that? I sized the buttons to fill the width of the sidebar, increasing the size of the click target, and I moved them to the top to simplify the positioning of the buttons, instead of having lots of magic numbers. (Once we go deeper into the template refactoring, we won't need absolute positioning at all, but I wanna make sure I know the specific tweak to make here.)

Also, it would be great if we could maintain consistent styling for the "Last Updated" label to keep everything looking cohesive.

I've adjusted the font size and padding of the label back to the what they used to be; my math when converting the pixels to relative units was a little off due to Bootstrap's resets. Was that the "consistent styling" you were referring to? I changed the label colour of the "Bright" theme to ensure sufficient colour contrast, but I get that it departs from the minified Bootstrap's pre-defined palette. Would changing it to black be sufficient, since we use it as an accent in the overall statistics section? If not, I'm happy to revert that specific change for now; I'll propose other solutions another time.

I also noticed that some of the colors in the date range appear slightly different across the various themes, so that might need a little tweaking as well.

Fixed!

Feel free to move it out of draft when you think it’s ready, and I’ll be happy to take another look.

Let me know what you think about the additional internationalisation keys. I'll put a hardcoded English label at the very least, but I think it'd be better to slot them into the language files; I just don't know the process for that, if one exists. Would we just slot them on the end?

@allinurl
Copy link
Owner

Thanks so much for making those changes, and apologies for the delay in posting this.

I've got a clarifying question for the menu icons: Would simply moving them lower, closer to their original position, be sufficient, or are you looking for something more than that?

Yes, simply positioning them at their original spot would be fine, just to maintain consistency with the alignment of the other headers. I agree that relying on "magic numbers" isn’t the ideal way to set them.

Was that the "consistent styling" you were referring to?

That should be fine. I think I was referring to the small padding on that label—my browser is rendering it slightly higher than vertically centered, but I’m okay with that if it's hard to center it. The only thing missing would be the color on the bright theme, e.g.,:

2024-11-25-173542_310x40_scrot
2024-11-25-173537_288x47_scrot

Regarding the i18n labels, as long as you add them to this file, they can then be passed to the report here. Let me know if that’s what you meant.

Thanks again!

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