Fix: Remove duplicate header logo anchor element

[dsmchen]

Because

There are 2 header logo anchor elements that are always shown on desktop/mobile. The anchor elements are functionally identical (same href and aria-label), the only difference being the contents.

This PR

• Axe the first anchor element and move the <img> inside the second anchor element.
• Drop block from the <img> classes since that's the default anyway.

Issue

Closes #5200

Additional Information

Pull Request Requirements

• I have thoroughly read and understand The Odin Project Contributing Guide
• The title of this PR follows the keyword: brief description of change format, using one of the following keywords:
  • Feature - adds new or amends existing user-facing behavior
  • Chore - changes that have no user-facing value, refactors, dependency bumps, etc
  • Fix - bug fixes
• The Because section summarizes the reason for this PR
• The This PR section has a bullet point list describing the changes in this PR
• I have verified all tests and linters pass after making these changes.
• If this PR addresses an open issue, it is linked in the Issue section
• If applicable, this PR includes new or updated automated tests

[mao-sz]

Hmmm, unfortunately the fix isn't as simple as that because it turns out the logo component is also used in

theodinproject/app/views/shared/_off_canvas_menu.html.erb

Line 49 in bc97027

<%= render LogoComponent.new(classes: 'h-12 w-auto') %>

for the mobile sidebar menu, and the current changes have both SVGs show.

@JoshDevHub @KevinMulhern any thoughts on preferred way forward?

The issue is having two anchors still focusable even if their contents are visually hidden.

Probably simplest way would be to move the hiding to the anchors themselves and not their images. So still have two anchors in the header but only one will be focusable at any one time. That would also require amending the logo component because the classes param only applies to the image contents, but we'd want to pass in classes for the image and the anchor separately so the anchor can be given hidden lg:block in the navbar and nothing in the mobile menu.

[JoshDevHub]

Footer also duplicates it on mobile:

This is because most callsites don't pass a default hidden class to the LogoComponent render: https://github.com/search?q=repo%3ATheOdinProject%2Ftheodinproject%20render%20LogoComponent&type=code

And that means the svg won't get hidden at smaller screen sizes (where the image will also be visible).

I'm not confident what the best thing to do is. I'm kinda curious why the code was originally written like it was. Basically:

There's a LogoComponent that renders an anchor wrapping an svg tag of the site logo

In the navbar, that LogoComponent was rendered while passing classes in that hide the svg by default and display at the lg breakpoint.

But also there's a plain link_to that essentially repeats the LogoComponent, but with an image_tag instead of inline_svg_tag, and that's what the navbar shows at small screen sizes and hides at larger ones.

Maybe Kev can say some more about it when he looks at this, but to me, why have the image at all? Is there something I'm missing about just using the plain old LogoComponent with inline_svg_tag on smaller screens in the navbar specifically? Because it seems to work fine in other places on the app.

[mao-sz]

Explored this a bit more with Josh earlier. We wonder if we can just drop the icon-only logo anchor entirely, and keep the long logo in all situations.

The only thing that would need tweaking would be some margin/gap stuff with the header nav at just above 767px (the sm/md cutoff) as for about ~40px above it, some of the nav items individually wrap and look off.
If that can be addressed, we can have the full logo looking nice all the way down to 768px with the navbar, then at 767px it definitely won't be a problem when the navbar changes to the hamburger menu.

@dsmchen I'll play around with this more when I get the time but in the meantime, if you want to play around with this too and explore not needing to hide/switch between icon-only and long logos, do feel free!

[JoshDevHub]

RE: my last post, Mao told me why there were two different icons lol.

If you can't get the wide icon to work right above the mobile breakpoint, I think I'm actually fine with a bit of a refactor. To my mind, what's going on here is that we would like LogoComponent to sometimes collapse to the "small" version of the logo. We could rewrite LogoComponent some to handle this variant, but I don't like that so much (open-closed principle). I'd prefer something like:

1. Small refactor to pull more of LogoComponent's behavior into methods. This will allow easier extension.
2. Create a CollapsibleLogoComponent that inherits from LogoComponent and supplies the variant behavior.
3. The navbar yanks out the image tag render and LogoComponent render and replaces it with a single render of CollapsibleLogoComponent. All other LogoComponent renders are left alone.

[mao-sz]

Do we need a collapsible logo in the first place? If we can successfully tweak the navbar spacing just above 767px breakpoint, then we could just keep the one logo in its full form since at 767px and below, the navbar disappears into the hamburger menu anyway (and IIRC, might have missed something), the "collapsible" behaviour is only used in the header because of the navbar

[JoshDevHub]

Do we need a collapsible logo in the first place? If we can successfully tweak the navbar spacing just above 767px breakpoint, then we could just keep the one logo in its full form since at 767px and below, the navbar disappears into the hamburger menu anyway

Sure. That's why I said "if we can't get it working with the wide logo above the breakpoint to mobile, we could do this other thing".

I do think it might be a bit of a challenge to get the wide logo looking good. If the user is signed out, the 'Getting Started' button takes up more space in the navbar than the user profile that's there when the user is signed in. This makes the navbar links' text wrap at widths up to ~930px, with compression stopping entirely at around 870px, leading to situations where the content starts to overflow the right edge of the screen. It's just a lot to cram in all together.

[mao-sz]

Ah, missed the "if" but also good point about the navbar if signed out.

[JoshDevHub]

Actually after thinking on this more, I think we can just refactor with ViewComponent slots. Sorry this unintentionally became a more involved problem @dsmchen. Here's the ViewComponent documentation to maybe help with some of the vocabulary I'm about to use.

But essentially, we could allow the LogoComponent to renders_one :small_icon (you can just do this with a lambda slot). If the small icon is present, the component should make sure it's hidden at larger screens and shown at smaller ones (and vice versa for the regular icon of course).

I'm not sure whereabouts in the curriculum you are or what your experience with Ruby/Rails is, so if everything I just typed is flying over your head, just let me know and I can offer more guidance.

[dsmchen]

I'm not sure whereabouts in the curriculum you are or what your experience with Ruby/Rails is, so if everything I just typed is flying over your head, just let me know and I can offer more guidance.

Hey @JoshDevHub, I'm on the JavaScript path, so your suggestion is beyond me. 😅 Perhaps we can open up this PR to others? Maybe someone on the Ruby on Rails path would like to work on this.

[JoshDevHub]

I can probably just do it myself really quick. It's kind of a tricky issue, probably even for Rails learners because we don't show ViewComponent in the curriculum.

Thank you for having this idea though!