Open heyainsleymae opened 3 weeks ago
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.
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.
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?
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.,:
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!
Some initial progress on #2742 that can be merged separately from the more intensive work like menus and tables
Most relevant changes
<ul>
and make each statistic an atomic live region<aside>
, all panels are inside of the<main>
<article>
with an appropriate programmatic namearia-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
fa-square
when not connected, changing to the same greenfa-circle
once a connection is established