Repository navigation
Add the header section - #4
Conversation
🔍 Code Quality Report
|
dabrorius
left a comment
dabrorius
left a comment
There was a problem hiding this comment.
I'm not sure about the class naming approach. I think it is leaning too much into flexibility direction, I'd rather keep it as simple as possible and extract variables only for setting we plan to be configurable.
| {%- if menu != blank -%} | ||
| {%- if app -%} | ||
| {%- capture element_app_mobile -%} | ||
| <div class="{{- class -}}__app"> |
There was a problem hiding this comment.
Why is class here a variable? Can we just do <div class="header__app"> instead?
There was a problem hiding this comment.
Just wanted to keep the code in one style 😁
I didn't manage to finish the apps yet. Just copied from the Chameleon theme. Will handle it later
| </i> | ||
| {%- endcapture -%} | ||
|
|
||
| <div id="{{- section_class -}}-{{- class -}}" class="{{- section_class -}}__{{ class }} {{ class }}"> |
There was a problem hiding this comment.
Do we plan to use this menu anywhere outside of the header?
I'm not sure that this approach is worth it, it makes code very hard to read. Can't we just have normal hard-coded classes everywhere?
If I wanted to change the styling of this DIV i have no idea where to start as the CSS classes could be anything.
There was a problem hiding this comment.
Considering the development experience of our existing themes, which share similar menu structures in at least three places, we can reuse this snippet in the footer and sidebar on the collection page. That's why there are menu_levels and use_opener keys, which allow the menu to be used anywhere.
I believe, it will also help us to make our work easier in monorepository
There was a problem hiding this comment.
Can we use the similar approach as for example Shopify Dawn theme header or mega menu? They have clearly defined classes.
The menu will always be a menu, right? So we can have something like this:
<div class="menu">
Then if we need to conditionally change styles when this menu is in footer, we can do something like:
<div class="menu{% if position=="footer" %} menu--footer{% endif %}
That way it would be easier to see what class combinations are supported and make sense.
Is this approach with flexible classes like class="{{- section_class -}}__{{ class }} {{ class }}" considered best practice in Liquid?
🔍 Code Quality Report
🎉 All checks passed! Ready for review. |
🔍 Code Quality Report
🎉 All checks passed! Ready for review. |
- Updated minification process for JavaScript files to include additional compression options. - Changed SVG build process to utilize a new optimization script instead of copying files directly. - Improved SVG icons for better structure and readability. - Added a new utility script to optimize SVG files, reducing file size and improving performance.
🔍 Code Quality Report
🎉 All checks passed! Ready for review. |
🔍 Code Quality Report
🎉 All checks passed! Ready for review. |
This PR introduces the header section.
Updated
.stylelintrc.jsonto enforce a stricter CSS class naming pattern, helping maintain consistent and readable class names across the codebase.Removed the unused code/files to keep the repository clean and focused on production code.