My Ada Sidebar

[jacbn]

A good chunk of these line changes are just whitespace from adjusting the structure of the sidebar-containing component!

[jacbn]

This has annoyingly gone through a lot of back-and-forth changes, but I have now moved the working sidebar inside a feature flag, with the intention of being able to just get this merged and future changes being much smaller and easier to review. I have also split the container refactor into its own PR

As is, this PR adds a sidebar to all the My Ada pages. There are a few changes still to make to the teacher side, perhaps the most notable being replacing the My Ada dropdown in the top menu with a button that takes you to the overview. There is also additional work required on the student side, the largest being a student overview page.

With this in mind, the work for the sidebar is obviously incomplete, but for the sake of this review it would be useful to:

• double check that no changes that are intended to be behind a feature flag affect the non-feature flag version;
• review the improved StyledSelect component + CSS;
• review the functionality (not really the contents, as this is subject to change) of the sidebar, on both mobile and standard screens;
• and lastly, I would really appreciate opinions on changing the breakpoint sizes on Ada. I am not convinced what I did here was necessarily the best approach, though a cleaner approach is significantly more work and would need to affect far more pages than the scope of this PR.

As a short summary of this last point, pages were built around md being a particular size; the reason breakpoints exist is so that content doesn't overflow or be noticeably wrapped. If we just add in a sidebar, however, the "effective" size for the page decreases because there is less space available for the page content, despite the screen width still being the same. The real fix for this is to move the entire page layout to containerised widths, i.e. if the content exceeds a breakpoint then it changes layout, rather than the screen. Unfortunately, this is not how Bootstrap works (w-md-50 for example always refers to the screen width) and basically our entire layout system uses Bootstrap. I really would like to make this change at some point, but this needs a lot of work. As a simpler alternative, I have just boosted the Bootstrap breakpoint values to include the expanded sidebar width (~220px). This obviously has drawbacks (pages use smaller breakpoints than they could when the sidebar is collapsed or not present) and is why I wonder if something else could be done. I do think, however, that since the sidebar is relatively small, the impact only affects particular devices, and using a mobile layout "too late" is much better than the other way around of using a desktop layout "too early".

[sjd210]

A few small things (so hopefully not too much more back-and-forth), largely to do with when the feature flag is disabled.

I assume based on conversation that we don't have a timeline for when this releases beyond the feature flag? Are we planning to have the student one in too first? Do we have any clue, or are we just left to wonder?

[sjd210]

I'd rather this be in the standard format created in #1825. It doesn't practically make much difference on the page itself (~1 pixel) but good for the sake of re-use or for if we want to make future changes.

Suggested change

	<svg width="24" height="24" viewBox="2 2 20 20" fill="none" xmlns="http://www.w3.org/2000/svg">
	<path d="M6 19H9V13H15V19H18V10L12 5.5L6 10V19ZM4 21V9L12 3L20 9V21H13V15H11V21H4Z" fill="#870D5A"/>
	</svg>
	<svg width="18" height="18" viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg">
	<path d="M 3,16 H 6 V 10 H 12 V 16 h 3 V 7 L 9,2.5 3,7 Z M 1,18 V 6 L 9,0 17,6 V 18 H 10 v -6 h -2 v 6 z" />
	</svg>

[jacbn]

Could you create some guidelines for doing this to icons, on the wiki perhaps? I do find it a little strange that we've chosen 18px x 18px as opposed to a more standard size – I appreciate this won't matter at all as it's getting scaled but for creating new icons / rescaling them from a design sheet this is an unusual default to have.

Something that I wish I understood a little better at the time is that e.g. the Ada icons are all designed within a 24px x 24px space, with lots of padding around each icon – but this is intentional so that all the icons look the same size. It's not really about actually being the same size as much as it is looking like they are. Take this screenshot from the sidebar designs, for example; none of these look out of place, but the group icon is a good 40% wider than the home icon; if we force all our icons to maximise their size without considering how much padding they need to be visually consistent, they start to look out of place. The groups icon is a great example:

(left – designs, right – implementation)

It's definitely subtle and not something I think most people would notice, but it's been annoying me for a while now: the groups icon just feels too small. Because there is also inconsistent scaling between icons, they also end up with different line widths, which isn't helping.

The biggest issue imo is simply having these icons come from so many sources. We have a component library per site and a bunch of old icons not really from anywhere that all use different spacings. I liked the consistency that removing all the spacing around icons gave us, but I'm now seeing problems like the inconsistent line width that I just don't know if we can solve easily.

Anyway, this is unrelated to the PR. We should be consistent with what we have for now, so happy to change.

[sjd210]

I'll create a wiki page for it 👍

It probably is more suitable to have this at a standard size - it's just not something I'd considered very thoroughly since as you say we always scale the image. In this case, the height/width ratio is 1.125 and it was scaled such that the svg path was 16px wide which made the SVG as a whole 18px, but it would've been perfectly valid to make the svg path width a decimal instead. That probably does make more sense.

---

As for the rest of your thoughts, I think that this is something that the new icon system can deal with reasonably, but this just hasn't been put into action (and we probably do need to think about it more first anyway). The idea behind these full-width icons is that we have full control over their size and spacing, rather than splitting the control between the code and the SVG itself. We're not really actually doing that now.

In this case, we could scale up the icon-group image to the width we want and then align it with margins. Something like:

.icon-group-wide {
  @include svg-icon('/assets/cs/icons/group.svg', calc(var(--icon-size) * 1.2), calc(var(--icon-size) * 1.2), var(--mask-size, 60%));
  margin: 0 calc(var(--icon-size) * (1 - 1.2)/2) !important
}

(This margin works to align any scaler ratio, just replacing the 1.2 with the new ratio. I did the maths :) ).

This results in the icon looking a lot more like the original image in those designs (once the ms-1 on the icons is also moved to the surrounding div):

This kind of structure could definitely be combined together to its own mixin, and this gives us the benefit that we can use both the original icon that matches the width of the other icons, and this wider one that feels more suitable here in this space (e.g. I think I prefer the standard width for the same symbol on the markbook).

Anyway, agreed that this is unrelated to the PR. I've got more thoughts that we can probably discuss at some later point, but for now I'll make a card documenting what we've said here.

[jacbn]

This works for sizing, agreed, but scaling up the icon in the code by different amounts per icon will mean the effective line widths between two icons that come from the same source (e.g. Ada component lib) will be different too. I can't really tell how noticeable this'll be without seeing some real examples; I doubt we'd ever scale more than about 1.3–1.4ish, but maybe with adjacent icons you'd notice..?

I bring this up now because I'm trying to fix the tick/cross icons and it's not going well 😭

[sjd210]

I'm not sure I understand the issue. Are you talking about line widths as in the widths of the lines within the SVGs themself? Because these do get scaled up when scaling the icon, like in my image above.

What tick/cross icons are you trying to fix? e.g. If you're looking at tick-rp and cross-rp, those have had different internal line widths forever (despite being from the same source) and weren't touched by the icon refactor. If that's a problem, it's a more fundamental one.

[jacbn]

Because these do get scaled up when scaling the icon

Sorry, yes, this is the problem. If say Ada give us multiple icons (looks like all theirs are Material Icons) that were all created with 1 arbitrary unit line width and we scale them differently, they won't have the same line width relative to each other, despite having been designed so. This will make certain icons look thicker than others when we render them, when the base icons themselves don't suffer from this.

I was looking at tick.svg and cross.svg (common/icons/), which aren't full-width but do have the same line width (looks like they are from Materials Icons). If we adjust the viewbox to "perfectly fit" both, the widths end up being slightly different, so when scaled to a fixed size via say icon-md, they now have different line widths.

[sjd210]

Not sure if this has been discussed, but it it intentional for the Markbook tab to become inactive when in a specific group/assignment and only active when on the Markbook overview? This is what the code is doing, and for me this feels counterintuitive - I'm still within the Markbook section, much like how in different tabs of the Account section are still within the Account. I'd prefer it stays active for any /my_markbook/... link

[jacbn]

It hasn't, but I fear this is bigger than a simple change owing to niche but accessible pages (e.g. test management) being at URLs which aren't prefixed by the initial one (e.g. /test/assignment/{id}/feedback not being under /markbook/). I'll bring this up with Ada rather than try to solve it here.

[sjd210]

Well in that case, the (potential) issue is that the sidebar isn't there at all - and that page can also be reached from either the Markbook OR Tests 🤔.
I think my preference would be that we change the URLs so that this is part of the /my_markbook/... path in the first place (redirecting any old ones) because I don't like this current path structure anyway, but let's see what Ada thinks.

[sjd210]

I can't find any other odd behaviour outside the feature flag, so I think this is good to merge and for any other work to be saved for the future.

That said, one piece of that future work is to investigate an issue with the sidebar not reaching the bottom of the page when the page is sufficiently small. Strangely I wasn't having this issue before dismissing the account verification banner OR while in inspect element, but it happens consistently otherwise. 🤷‍♀️

I'll make this a separate card and you (or someone else!) can deal with it how you like later.