Snippetize header menu components

[kmeleta]

PR Summary:

Why are these changes introduced?

Improves developer ergonomics around the header by splitting some related components into their own snippets.

What approach did you take?

I split the respective code for all 3 of the menu types into their own snippets. I also captured the search modal markup into a snippet since this code is fairly verbose and also duplicated in header.liquid. The search modal needs to avoid duplicate ids so I took the same approach we used in the product-media-gallery snippet for that. Otherwise, no code has been changed in any of these components.

Other considerations

It would be great if any of the redundant nested lists used in the menus could be consolidated but I don't make an attempt to consider or implement any changes for that here.

Testing steps/scenarios

This should be a low risk change but we should ensure everything works exactly as it did before.

• Test all 3 header menu types for regressions (mobile and desktop)
• Test using several logo positions
• Test predictive search for regressions (particularly using top-center logo position)

Demo links

• Editor

Checklist

• Added PR summary for release notes
• Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
• Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
• Followed theme code principles
• Linted with Theme Check
• Tested on mobile
• Tested on multiple browsers
• Tested for accessibility

[ludoboludo]

Looking good 👍 Just a couple nitpicks

[ludoboludo]

👍

[kmeleta]

Resolved minor conflicts from #2238. Wasn't bad, but probably double check nothing got mixed up.

[metamoni]

Looking good 🎉

[stufreen]

Looks good with one question

[stufreen]

It took me a second to understand what this was doing. What do you think about something more transparent like search_input_id: {String} or append_id: {String}? That will also let you have 3+ if you really wanted.

What's the search input ID used for anyhow, just the label?

[kmeleta]

It's likely only used for the label. I went back and forth on the first snippet that needed a duplicate like this. I originally did use append_id so if you feel strongly I'm happy to do that. I added a brief decision log entry in that PR that calls out that option. But my reasons were..

• Adequate and a simpler api to use more of a boolean approach.
• Nothing depends on our snippet apis so we can start simpler and update it anytime if needed.
• It's not necessarily meant to take the place of an "id" prop to support multiple instances in any number of sections or blocks, it represents a specific use where we're having to render one "instance" twice specifically to maintain proper dom order for a mobile and desktop layout.
• I would hope we can avoid needing 3 rendered search instances in the header.

Going further with it though, there could certainly be a case made for abstracting only the combobox portion out of the current header-search snippet instead and reusing that in the main-search. In such a case, we would definitely want to allow a fully custom id to be passed through to it. Any thoughts on that? I was motivated more to reduce noise in the header more than anything here, but technically if we were limiting ourselves to only one layer of abstraction (because this is liquid) I could see that be the more logical choice.

[stufreen]

I don't feel that strongly either way but is_duplicate just seems a little obscure. I didn't know what the param did until I searched the code to see where it was used. I'll leave it to you but input_id would be more obvious to me I think -- I'd assume it was an id getting passed on.

I wonder if we even need the id and for attributes if we could achieve the same thing by having the label as the parent of the input? Probably OOS here though.

[kmeleta]

I'm not sure if my point was received, but this instance definitely doesn't make as strong of a case for that prop pattern as the other snippet I was matching so I decided to adjust as you suggest.

I wonder if we even need the id and for attributes if we could achieve the same thing by having the label as the parent of the input?

Well I definitely don't want to refactor every input in dawn to avoid a naming discussion regardless haha, but I imagine a major reason we chose the label for input pattern is so the label could access the :focus context of the input element in css (input:focus ~ label). Off hand I'm not aware of every detail considered in original decision, but I'm sure it wasn't made by accident or with ignorance to the label parent option.